GNOME Bugzilla – Bug 742747
preview-mode: when in fullscreen, ESC should not bring to overview
Last modified: 2015-10-12 15:26:59 UTC
First of all, there is no way to fullscreen or !fullscreen the application in overview mode, the application can only be made fullscreen in preview mode which makes sense as it might be design decision. You can fullscreen the application in preview mode by pressing F11. To !fullscreen the application, currently we have to press F11 again, pressing ESC should also bring it back to !fullscreen mode. Application can get stuck in following situation. Currently when in preview mode, pressing ESC brings the application to overview mode after which there is no way to !fullscreen the application (pressing F11 doesn't work here, ESC works neither), it stucks in fullscreen forever in overview mode unless you go again to preview mode and press F11 to switch to !fullscreen mode.
Created attachment 298801 [details] [review] ESC on fullscreen preview brings back to !fullscreen preview, ESC on preview brings back to overview Tested it. Couldn't find any bugs introduced due to this. Please review.
Review of attachment 298801 [details] [review]: Good work overall ! However few points to be noted. Your commit message subject is too long. "First line (the brief description) must only be one sentence and should start with a capital letter." It should be also be less than 72 characters. See: https://wiki.gnome.org/Git/CommitMessages Secondly, lets use a separate patch for new feature additions. You should only solve one problem in one patch. Besides that, I also noted some minor nitpicks as mentioned below. ::: src/photos-main-window.c @@ +229,3 @@ +{ + PhotosMainWindowPrivate *priv = self->priv; + gboolean fullscreen; nitpick: We use space to separate variable declarations with rest of the code. Lets follow the convention. @@ +234,3 @@ + { + gtk_window_unfullscreen (GTK_WINDOW (self)); + photos_mode_controller_set_fullscreen(priv->mode_cntrlr,FALSE); nitpick: Use space to separate arguments i.e use space after comma. nitpick: fix the indentations. @@ +258,3 @@ case GDK_KEY_Escape: + photos_main_window_handle_esc_key(self); + handled=TRUE; nitpick: Please note that there should be a space before the opening bracket. Similarly, there should be a space to both sides of =, as in other assignments.
Thanks for the review. For the feature addition, the patch depends on the earlier patch. Also, should I open new bug for the feature addition?
(In reply to Kunaal Jain from comment #3) > Thanks for the review. For the feature addition, the patch depends on the > earlier patch. Also, should I open new bug for the feature addition? Yeah, no problem if your patch depends on earlier patch. You can separate the feature addition part and attach it here itself.
Created attachment 298809 [details] [review] ESC on fullscreen preview brings back to !fullscreen preview When in fullscreen and preview mode, pressing ESC brings back to overview mode and gets stuck in fullscreen mode there. This fixes it.
Created attachment 298812 [details] [review] ESC on preview brings back to overview This patch depends on the patch submitted earlier.
Summary of the two patches : Instead of handling the ESC key in photos_main_window_handle_key_preview function, I created a function to handle ESC key when pressed on preview mode. If fullscreen, then unfullscreen. Otherwise go back to overview mode.
Review of attachment 298809 [details] [review]: See: https://wiki.gnome.org/Git/CommitMessages Your description should be wrapped in under 72 characters. From the link : "Each line in main description must not exceed 72 characters (there is no limit on number of lines)."
Created attachment 298815 [details] [review] ESC on fullscreen preview brings back to !fullscreen preview When in fullscreen and preview mode, pressing ESC brings back to overview mode and gets stuck in fullscreen mode there. This fixes it
Review of attachment 298815 [details] [review]: Sorry, didn't notice it earlier. The commit message (first line) doesn't end with a dot (so remove '.' after "preview") Sorry for being too pedantic, these are given in guidelines. :)
Created attachment 298818 [details] [review] ESC on fullscreen preview brings back to !fullscreen preview When in fullscreen and preview mode, pressing ESC brings back to overview mode and gets stuck in fullscreen mode there. This fixes it
Created attachment 298819 [details] [review] ESC on fullscreen preview brings back to !fullscreen preview When in fullscreen and preview mode, pressing ESC brings back to overview mode and gets stuck in fullscreen mode there. This fixes it
No its alright. My bad. I should have read them carefully.
Other issue, is not possible to exit from the fullscreen mode if you are in overview mode. You have to select a pic and then if you press F11 let you exit from the fullscreen, but not before.
(In reply to Miguel Vaello Martínez from comment #14) > Other issue, is not possible to exit from the fullscreen mode if you are in > overview mode. You have to select a pic and then if you press F11 let you > exit from the fullscreen, but not before. This is a terrible usability issue, please file it as a separate bug.
(In reply to Pranav Kant from comment #0) > First of all, there is no way to fullscreen or !fullscreen the application > in overview mode, the application can only be made fullscreen in preview > mode which makes sense as it might be design decision. The expected behaviour is: - Unless you are forcing a fullscreen through the window manager (see bug 725508), you can only fullscreen while in the preview. - F11 will toggle the fullscreen state, but leave you in the preview. - Only when fullscreened, ESC will take you back to the overview and unfullscreen. - Alt+<back> will always take you back to the overview and unfullscreen if needed. > You can fullscreen the application in preview mode by pressing F11. To > !fullscreen the application, currently we have to press F11 again, pressing > ESC should also bring it back to !fullscreen mode. > > Application can get stuck in following situation. > Currently when in preview mode, pressing ESC brings the application to > overview mode after which there is no way to !fullscreen the application > (pressing F11 doesn't work here, ESC works neither), it stucks in fullscreen > forever in overview mode unless you go again to preview mode and press F11 > to switch to !fullscreen mode. There were some bugs in our fullscreen handling code (see bug 725508 and bug 756131) which had earlier been fixed in gnome-documents (makes sense because they share the same origin). I have now cherry-picked them to master, gnome-3-18 and gnome-3-16. Therefore, things should be working as expected. If you think the expected behaviour is not reasonable, then I would suggest filing a bug against gnome-documents for wider discussion because that is where the behaviour originated from and I would like to keep them consistent. If you think that it is not behaving as expected, then please file a new bug against gnome-photos.
*** This bug has been marked as a duplicate of bug 756131 ***