GNOME Bugzilla – Bug 681540
Direct manipulation of the wallpaper
Last modified: 2013-02-18 20:24:00 UTC
Something that has been discussed for some time and that has recently come up in user testing is that some new users, especially those with a Windows background will expect to be able to change the wallpaper by right clicking or long pressing on it directly. We discussed this already once in bug 661998. I had some reservations about that. Primarily * It seems a bit weird to have a menu with one item in it * We expected there to be a built-in Photos app * We expected there to be an improved Background settings panel (there is code languishing in the wip/background-content branch - someone help!) * We weren't sure if we would offer the option to change wallpaper in the initial setup. Some things are different now but the problem remains. I think we probably do want to have a way to directly manipulate / change the wallpaper. The question is how? * Can we make a menu item not suck with only one thing in it? * Are there other options that make sense without just being tossed in? * Is there an even more direct or less mediated way to change the wallpaper? * Is just starting the Background settings an option? * Something even better?
* Is just starting the Background settings an option? You mean launching that on right-click/long-press? I'd say it's worth a try ...
(In reply to comment #0) > Something that has been discussed for some time and that has recently come up > in user testing is that some new users, especially those with a Windows > background will expect to be able to change the wallpaper by right clicking or [..] Are those user testing results available? I'd like to link to it. Doesn't have to be perfect.
(In reply to comment #0) [...] > * Are there other options that make sense without just being tossed in? [...] The wallpaper fills the screen background which can be seen as as a metaphor of the screen itself. Also, users with a Windows background may expect to be able to change screen resolution from there too. I wonder whether users would look for brightness/screen/display settings in the screen background. It would probably need some user testing.
It is better to have a menu with a single item than have no menu at all. It would be cool to see this get fixed.
That's what bug 661998 did.
(In reply to comment #5) > That's what bug 661998 did. History repeating itself. :) I'll check with Jon.
Just a random dummy idea, but... if one user clicks on the background, could we imagine to display some items on it. I might have to illustrate this with some picture, but basically drawing options on the background, with symbolic icons, related to changing background / screen settings. It's more or less the same idea, but painting a big transparent menu over the background might be less weird than a Gtk menu poping on the cursor, and we could use all the available space, to have explicit options shown. This would imply to minimize any window there.
How about opening the System Settings at the first start of the system?
By right clicking on the background of the desktop a message could pop from notification area letting the user know where he could change the background. It will be more gnomish and make user get used to the notification system in GS plus he will learn where are the settings are.
Allan: did you check with Jon? I mostly agree with its arguments. The idea of Denis would seems to be good but for only for a long press on touchscreen. I would not want to break the association right-click == popup menu. So we could: - on long press, offer a notification to propose going to the background settings or directly go to it (but I feel that could become bothersome in case of "manipulation error" ... both are maybe bothersome in this case, it's your call) - on right click, offer a shell-style context menu "Background Settings" Easy fix but hard decision :)
Apologies for the delay here. I spoke about this with Jon and Jimmac today. We agreed to go for a menu (activated either through long press or right click) with two items: "Change Background..." and "System Settings". The latter is included because there is a strong affinity between the background and the system, and because Windows users are familiar with using the wallpaper context menu to access some system settings.
(In reply to comment #11) > [...] We agreed to go for a menu [...] Quick clarification - we are talking about a shell menu rather than a GTK+ one, right?
(In reply to comment #12) > (In reply to comment #11) > > [...] We agreed to go for a menu [...] > > Quick clarification - we are talking about a shell menu rather than a GTK+ one, > right? Right.
Sounds like the design may have already been decided but how about on right click a box ("box" might be the wrong term here - I mean a small window in the shell style e.g. like the menus but not an actual menu) appears. In this 'box' have a click-able image of the background, like in the normal background settings. Clicking the image would launch the usual background selector, just as you had clicked on the mini background image in the normal background settings. To me this is a bit more interesting than simply a 2-item menu. Also, with a menu, more options may creep into it at some stage. Suddenly it's a 3,4,10 item menu etc. Maybe not in gnome-shell proper but distributions may decide they know best and add many options (which would be bad :)). This solution also reduces the number of clicks required - One right click to show the mini click-able background, then one to click on it to change it. To me this seems more fun than right click, select menu item, click mini background image. If they want to go to system settings they should use the shell normally. Windows doing something a certain way doesn't justify a decision. What other decision in gnome-shell is dictated by what windows does? In my opinion the strong affinity between the background and the system should be replaced by a strong affinity between the overview and the system. We want users to use the overview.
I think there's been discussions about moving the background drawing from gnome-settings-daemon to gnome-shell... I think this might have implications for this bug. Would that mean the shell would use an actor for the desktop background?
It already does. MetaBackgroundActor. Things should be no different in this case vs. the previous.
Created attachment 232058 [details] [review] screen: Select for ButtonPress/ButtonRelease on the guard window This gives us an easy place to allow users to right-click or long-press on the wallpaper. As only one client can select for ButtonPress/ButtonRelease, the guard window is a good substitute.
Created attachment 232059 [details] [review] compositor: Set the background actor to be reactive by default Combined with the previous patch, this gives us an easy way to connect for events on the wallpaper.
Created attachment 232060 [details] [review] compositor: Install the stage window on events where we don't have a window This allows events generated for the root window or guard window to be picked up by Clutter as if they were events for the mutter stage.
Created attachment 232061 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it.
These patches are on top of bug 690590 for simplicity in implementation.
(In reply to comment #21) > These patches are on top of bug 690590 for simplicity in implementation. Let's make it a dependency then.
Jasper: argh, I did not had the time to do it (not that much time recently) although you gave me the full way forward and took time to explain. Thanks for jumping in and do it.
Hello everybody. I think (from the user perspective) that it would be nice to have the "menu" customizable. The "standard" options eg. changing wallpaper and System Settings could be separated from "user defined" by a horizontal rule (like the one in the menu that pops when you click your user name in Shell) or something similar. New entries can be controlled by JSON, maybe? Or make it available for extensions developers.
Created attachment 232101 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it. Fix some stuck grabs, make easier to modify from extensions, and add a proper gap style so that the menu doesn't slide down from above the mouse cursor.
Jasper, can you rebase those patches to master?
(In reply to comment #26) > Jasper, can you rebase those patches to master? After I'm done rebasing barriers and testing/reviewing GrabKey patches, and working on random stuff at the DX hackfest, like my giant set of changes to introspection for better documentation.
Created attachment 235027 [details] [review] screen: Select for ButtonPress/ButtonRelease on the guard window This gives us an easy place to allow users to right-click or long-press on the wallpaper. As only one client can select for ButtonPress or ButtonRelease on the root window, the guard window is a good substitute.
Created attachment 235028 [details] [review] compositor: Set the background actor to be reactive by default Combined with the previous patch, this gives us an easy way to connect for events on the wallpaper.
Created attachment 235029 [details] [review] compositor: Install the stage window on events where we don't have a window This allows events generated for the root window or guard window to be picked up by Clutter as if they were events for the mutter stage.
Created attachment 235034 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it.
Is this ready to land ?
If background settings go here because that's where people are used to them being in Windows, should we also put some shortcuts to other display-related settings? Change resolution, change multi-monitor modes, that sort of thing?
Review of attachment 235029 [details] [review]: So I didn't like this at first. My thought was "Don't we reshape the stage input region already to allow events we want to come through?" and I talked to Jasper about that. Truth is, we only reshape the input region to include space for the panel and other chrome. Doing anything more than that would require resetting the input region every time a window moved, which could carry performance implications. There's apparently a possible longer term plan to put surrogate X windows in place behind all chrome elements anyway, in which case (I guess) the stage won't be getting any input events directly anymore. ::: src/compositor/compositor.c @@ +757,3 @@ +static void +install_stage_window (MetaCompScreen *info, maybe spoof_event_as_stage_event or rewrite_event_window_for_stage or something? @@ +763,3 @@ + + if (event->type == GenericEvent && + event->xcookie.extension == meta_display_get_xinput_opcode (display)) we seem to use display->xinput_opcode everywhere else in-tree. @@ +770,3 @@ + switch (input_event->evtype) + { + case XI_Motion: you don't select for this. @@ +776,3 @@ + Window xwin = clutter_x11_get_stage_window (CLUTTER_STAGE (info->stage)); + XIDeviceEvent *device_event = ((XIDeviceEvent *) input_event); + device_event->event = xwin; This is a little unsavory. I guess we should have a fat comment at the top of this function explaining why this function is needed.
Review of attachment 235027 [details] [review]: Sure. Do you need XI_MOTION too ? I'm guessing not.
Review of attachment 235028 [details] [review]: ok
Review of attachment 235034 [details] [review]: ::: data/theme/gnome-shell.css @@ +562,2 @@ .panel-menu { + -boxpointer-gap: 4px; probably better as a separate commit, but sneaking in a little fix like this is pretty harmless I suppose. ::: js/ui/backgroundMenu.js @@ +33,3 @@ + + _startControlCenter: function() { + let app = Shell.AppSystem.get_default().lookup_app('gnome-control-center.desktop'); probably needs to check Main.sessionMode.allowSettings (and same for background panel). Maybe just use addSettingsAction ? @@ +56,3 @@ + } else { + // Don't use action.coords as it's the coords of the press, + // not the release. So is this a workaround for bug in Clutter.ClickAction ? Can you use Clutter.get_current_event() instead of the round trip?
(In reply to comment #37) > Can you use > Clutter.get_current_event() instead of the round trip? Looks like no: if (result) { priv->long_press_id = clutter_threads_add_timeout (timeout, click_action_emit_long_press, action); }
oh thinking about this more, with a long press event, the ui reacts before the release anyway. Is your concern that the user might drag their finger a little under the drag threshold so it moves slightly but still counts as a long press?
(In reply to comment #39) > Is your concern that the user might drag their finger a little under the drag > threshold so it moves slightly but still counts as a long press? We weren't tracking the drag (XI_Motion is missing -- good catch) so maybe it's better when Clutter gets the events it's supposed to.
Created attachment 236173 [details] [review] theme: Fix style
Created attachment 236174 [details] [review] popupMenu: Treat a menu will all invisible menu items as empty As an example, a menu that has only settings actions might be "empty" if allowSettings is false.
Created attachment 236175 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it.
Created attachment 236176 [details] [review] screen: Select for pointer events on the guard window This gives us an easy place to allow users to right-click or long-press on the wallpaper. As only one client can select for ButtonPress or ButtonRelease on the root window, the guard window, being a screen-sized window, is a good substitute.
Created attachment 236177 [details] [review] compositor: Set the background actor to be reactive by default Combined with the previous patch, this gives us an easy way to connect for events on the wallpaper.
Created attachment 236178 [details] [review] compositor: Install the stage window on events where we don't have a window This allows events generated for the root window or guard window to be picked up by Clutter as if they were events for the mutter stage.
Review of attachment 236173 [details] [review]: +
Review of attachment 236174 [details] [review]: good idea.
Review of attachment 236175 [details] [review]: cool ::: js/ui/backgroundMenu.js @@ +17,3 @@ + this.parent(source, 0, St.Side.TOP); + + let item; don't think you need this anymore @@ +25,3 @@ + + Main.uiGroup.add_actor(this.actor); + this.actor.hide(); Would be more conventional for the caller to hide it wouldn't it? @@ +43,3 @@ + // Don't use action.coords as it's the coords of the press, + // not the release. + let [x, y, mods] = global.get_pointer(); Didn't you say you didn't think you were going to need this anymore after fixing the event masks?
Review of attachment 236176 [details] [review]: ::: src/core/screen.c @@ +586,3 @@ + * Note that we also select events on the guard window. We do this + * as a replacement for the root window, as only one client can + * select pointer events on it, and we best not abuse that priviledge. Note sure it's really an abuse. I think that's traditionally how window managers have done it in days of yore. You misspelled privilege.
Review of attachment 236177 [details] [review]: +
Review of attachment 236178 [details] [review]: ::: src/compositor/compositor.c @@ +815,3 @@ +/* Clutter makes the assumption that there is only one X window + * per stage, which is a valid assumption to make for a generic + * application toolkit. As such, it will any events sent to the "it will ignore any events" @@ +816,3 @@ + * per stage, which is a valid assumption to make for a generic + * application toolkit. As such, it will any events sent to the + * a stage that isn't its X window. As the kind of application "that aren't specifically for" @@ +817,3 @@ + * application toolkit. As such, it will any events sent to the + * a stage that isn't its X window. As the kind of application + * mutter is, that's just not going to happen. For various reasons, I'd probably just drop the sentence. I don't understand it, but I don't think it adds anything to the paragraph. One thing to keep in mind is the guard window and the stage window aren't even in the same hierarchy. One is a child of the COW, the other isn't. @@ +821,3 @@ + * stage window, like the guard window, and use them for our own + * purposes. This function tells a little white lie to Clutter, + * making it think that it recieved a window on the stage window, s/a window/the passed in event/ @@ +822,3 @@ + * purposes. This function tells a little white lie to Clutter, + * making it think that it recieved a window on the stage window, + * allowing it to process it normally. s/allowing it to to process it/effectively proxying events to clutter actors that are positioned outside the stage's input region/ (or something) @@ +902,3 @@ + spoof_event_as_stage_event (info, event); + we should probably only do this for specific windows, right? This function is called for all events, I think.
Created attachment 236198 [details] [review] screen: Select for pointer events on the guard window The guard window is effectively the background window, as it sits in between live windows and minimized windows. This gives us a nice easy place to allow users to allow users to right-click or long-press on the wallpaper.
Created attachment 236199 [details] [review] compositor: Spoof events on the guard window This allows events generated for the guard window to be picked up by Clutter as if they were events for the mutter stage.
Attachment 236173 [details] pushed as 498bc21 - theme: Fix style Attachment 236174 [details] pushed as 30179bb - popupMenu: Treat a menu will all invisible menu items as empty
Created attachment 236200 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it. -- It still has issues: 1. Press right-click 2. Drag across the screen 3. Release No idea what other toolkits or OSes do (use release coords? cancel the action after going too far?) but I think Clutter's behavior isn't the most useful. I'll talk it over with Emmanuele tonight.
Review of attachment 236175 [details] [review]: ::: js/ui/backgroundMenu.js @@ +25,3 @@ + + Main.uiGroup.add_actor(this.actor); + this.actor.hide(); This was stolen from shellEntry.js. This looks like the traditional way to do popup menus.
Created attachment 236214 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it.
Created attachment 236217 [details] [review] window-actor: Set every window actor to be reactive Now that the background actor is reactive, this means that clicks on the window group part of the stage, even when they're on an X window, will be registered as the background actor, as all of the other children of the group aren't reactive. This can happen when a plugin takes a modal grab, for instance. Regression found -- take a modal grab (by opening a popup menu, for instance) and then click on the stage. The event gets sent to the background actor since it's the only reactive window in that area.
(In reply to comment #56) > It still has issues: > > 1. Press right-click > 2. Drag across the screen > 3. Release > > No idea what other toolkits or OSes do (use release coords? cancel > the action after going too far?) but I think Clutter's behavior > isn't the most useful. I'll talk it over with Emmanuele tonight. Looking at the code it's supposed to use ClutterSettings:dnd-drag-threshold to know when the user has dragged instead of pressed (or the :long-press-threshold property if set). I wonder if the default threshold is too large, or if that clutter code has a bug, or if it's some side effect of the event spoofing.
(In reply to comment #60) > Looking at the code it's supposed to use ClutterSettings:dnd-drag-threshold to > know when the user has dragged instead of pressed (or the :long-press-threshold > property if set). > > I wonder if the default threshold is too large, or if that clutter code has a > bug, or if it's some side effect of the event spoofing. Is that true for right-click, or just long-press?
looks like it doesn't call clutter_event_get_coords from the BUTTON_RELEASE handler.
Review of attachment 236198 [details] [review]: +
Review of attachment 236199 [details] [review]: +
Review of attachment 236217 [details] [review]: +
Review of attachment 236214 [details] [review]: seems to work okay mostly (modulo fallout from bug 690577 and one weirdness below) ::: js/ui/backgroundMenu.js @@ +38,3 @@ + function toggleMenu(action) { + if (actor._backgroundMenu.isOpen) { + actor._backgroundMenu.close(BoxPointer.PopupAnimation.FULL); This part doesn't seem to work. if i right-click when a menu is already open it moves the menu, doesn't close it. Not sure the current behavior is better then the implied behavior, but it does suggest there's a bug somewhere (isOpen not ever getting set to true maybe?) or that i'm misunderstanding something.
Attachment 236177 [details] pushed as f5de1c7 - compositor: Set the background actor to be reactive by default Attachment 236198 [details] pushed as b0774d7 - screen: Select for pointer events on the guard window Attachment 236199 [details] pushed as 5e9621e - compositor: Spoof events on the guard window Attachment 236217 [details] pushed as 491c5b6 - window-actor: Set every window actor to be reactive Le'ts get these ACN patches out of the way.
Created attachment 236662 [details] [review] backgroundMenu: Add a context menu on the background actor Allow users to change their wallpaper and launch System Settings from it.
Review of attachment 236662 [details] [review]: k. we still don't have exactly the behavior we want, and we still have the weirdy query_point thing, but doesn't matter.
Attachment 236662 [details] pushed as fae838b - backgroundMenu: Add a context menu on the background actor