GNOME Bugzilla – Bug 559024
[PATCH] Make seek or Ctrl/Shift show controls in fullscreen
Last modified: 2008-12-08 11:41:15 UTC
Right now, when Totem is in fullscreen and you seek with the arrow keys, there's no indication what the current position is or the new position (other players have an OSD for this). When seeking with the scroll wheel though, the fullscreen controls are shown. Also, there's currently no way to just show the fullscreen controls with the keyboard only. IMO this should be changed (patch follows).
Created attachment 121842 [details] [review] make seek or Ctrl/Shift show controls in fullscreen With this patch, the fullscreen controls are shown when seeking with the keyboard. Also, when just pressing Ctrl or Shift, the controls are also shown. This is useful when one wants to know the current position and the keyboard is nearer/easier than the mouse. Also, it's nice that when seeking with Shift+Right for example, the controls are shown as soon as Shift is pressed, making it possible to get a better "feel" for the seek because the progress bar is visible before the seek. It's almost like Totem knows that I want to seek and helps me with it :).
(PS: the patch above also adds a missing break)
There's no need to use the braces for the if statements, since there's only one line in the blocks. Other than that it looks good to me. Bastien?
The first hunk looks fine, the second one not so. I'd rather: - we added a function that mentioned clearly the popups were going to show (rather than (ab)using totem_fullscreen_motion_notify()) - the popup when Ctrl and Shift and arrows is pressed was called from where the seek is done (otherwise you're going to popup when no gnome-screensaver is available and we use the old X way by pressing shift keys for queen and country).
Ok, I'll come up with a new patch as soon as I have the time. Bastien: I agree with the first point and will change the patch. But I don't quite understand what you mean with the second point. Do you mean that it's a bad idea to pop up the controls as soon as Ctrl or Shift are pressed? What do you think would be a good key combination for just showing the controls (except for moving the mouse)?
I meant you should show the popups in the specific cases in totem_action_handle_key_press(), eg. GDK_Right, GDK_Left, etc.
Created attachment 123800 [details] [review] second version of patch (incorporates feedback) So here's a new patch which incorporates your feedback. There's a new function in totem-fullscreen.c called totem_fullscreen_show_popups, which does everything totem_fullscreen_motion_notify did before except showing the cursor (not sure about this one). The call to totem_fullscreen_show_popups is moved to totem_action_handle_key_press. I also refactored that part to remove code duplication. There are some still some calls to totem_fullscreen_motion_notify left in totem.c, maybe they can also be replaced with totem_fullscreen_show_popups (but that would slightly change the behaviour because it doesn't show the cursor). Off-topic question: Why should one use "if (bool != FALSE)" instead of just "if (bool)"?
If you want to go one step further, totem_fullscreen_motion_notify() should be marked private, and totem_fullscreen_show_popups() have a "show_cursor" argument. + case GDK_Shift_L: + case GDK_Shift_R: + case GDK_Control_L: + case GDK_Control_R: That needs to go. Rest looks fine. (In reply to comment #7) <snip> > Off-topic question: Why should one use "if (bool != FALSE)" instead of just "if > (bool)"? Because it allows us to see the type of "bool" without needing to search the sources. And the != FALSE is because TRUE is defined as !FALSE, and FALSE to 0. So TRUE could have any value that's not zero (although in reality it's probably not the case).
(In reply to comment #8) > If you want to go one step further, totem_fullscreen_motion_notify() should be > marked private, and totem_fullscreen_show_popups() have a "show_cursor" > argument. Ok, I'll do that. > + case GDK_Shift_L: > + case GDK_Shift_R: > + case GDK_Control_L: > + case GDK_Control_R: > > That needs to go. Rest looks fine. Do you mean the whole hunk? The reason behind it is to enable a user to just show the controls by using the keyboard. At the moment it's only possible with the mouse or with seeking, which is not very comfortable when one just wants to see the current time. Do you have an other key in mind for that, or maybe we could also show the controls when pressing space? > (In reply to comment #7) > <snip> > > Off-topic question: Why should one use "if (bool != FALSE)" instead of just "if > > (bool)"? > > Because it allows us to see the type of "bool" without needing to search the > sources. And the != FALSE is because TRUE is defined as !FALSE, and FALSE to 0. > So TRUE could have any value that's not zero (although in reality it's probably > not the case). Thanks for the explanation, I already suspected it was some kind of C peculiarity.
(In reply to comment #9) > (In reply to comment #8) > > If you want to go one step further, totem_fullscreen_motion_notify() should be > > marked private, and totem_fullscreen_show_popups() have a "show_cursor" > > argument. > > Ok, I'll do that. > > > + case GDK_Shift_L: > > + case GDK_Shift_R: > > + case GDK_Control_L: > > + case GDK_Control_R: > > > > That needs to go. Rest looks fine. > > Do you mean the whole hunk? The reason behind it is to enable a user to just > show the controls by using the keyboard. At the moment it's only possible with > the mouse or with seeking, which is not very comfortable when one just wants to > see the current time. Do you have an other key in mind for that, or maybe we > could also show the controls when pressing space? In lib/totem-scrsaver.c, we already use XK_Alt_L and XK_Alt_R to avoid the screensaver triggering when gnome-screensaver isn't there. I'd rather you added this functionality in another bug, probably using GDK_D (for Display), and added support for this functionality through the totem_remote_* stuff and the LIRC plugin. Rest looks fine to commit now.
Created attachment 123997 [details] [review] third version of patch Ok, so here's the newest version of the patch. totem_fullscreen_motion_notify is now private and the code to show the controls with Ctrl/Shift is removed from the patch.
Looks good. Do you have an SVN account?
No, I don't have one.
(In reply to comment #13) > No, I don't have one. Then please create a new request at: https://mango.gnome.org/new_account.php Following the instructions at: http://live.gnome.org/NewAccounts I'll commit this patch in the meanwhile. 2008-12-08 Bastien Nocera <hadess@hadess.net> * src/totem-fullscreen.c (totem_fullscreen_motion_notify), (totem_fullscreen_show_popups): * src/totem-fullscreen.h: * src/totem.c (totem_action_remote), (on_mouse_click_fullscreen), (totem_action_handle_key_press), (totem_action_handle_scroll): Show the popup when seeking with the keyboard in fullscreen, patch from Robin Stocker <robin@nibor.org> (Closes: #559024)