GNOME Bugzilla – Bug 668105
Many page menu items are available on a blank page
Last modified: 2012-01-23 17:15:05 UTC
When we start Web we show a blank page. On this page there are a lot of items in the page menu that are still available and that make no sense. For example: * Save As* * Page Setup * Print* * Send* * Find* * Add Bookmark * Encoding * Page Source
Could we turn EphyWebView ephy_web_view_is_blank into a property and listen to notify::is_blank? Sounds reasonable?
(In reply to comment #1) > Could we turn EphyWebView ephy_web_view_is_blank into a property and listen to > notify::is_blank? Sounds reasonable? Sounds reasonable to me.
Created attachment 205842 [details] [review] ephy-window: sync page actions with is-blank property Add ::is-blank property to EphyWebView and update EphyWindow to sync some of the page menu actions with it. There's no point in enabling save/reload/bookmark/etc on about:blank.
Review of attachment 205842 [details] [review]: I think it needs a few changes, but it's a great change, thanks! ::: embed/ephy-web-view.c @@ +1310,3 @@ + * + * %TRUE if the EphyWebView is blank. Usually meaning that it has + * about:blank loaded, as new tabs and windows do. I think I prefer something like "Whether the view is showing the blank address." and leave it at that. Soon enough new tabs/windows will show the overview, so better not to mention that. @@ +2521,3 @@ priv->is_blank = address == NULL || strcmp (address, "about:blank") == 0; + g_object_notify (object, "is-blank"); I know in theory it's ok to notify if the value has not changed, but I think I'd rather write this so it only happens when the value changes. So, from blank->non blank and viceversa. @@ +2569,3 @@ title = g_strdup (EMPTY_PAGE); priv->is_blank = TRUE; + g_object_notify (G_OBJECT (view), "is-blank"); Hrm, two notifies! Probably we should have an internal ephy_web_view_set_is_blank where we centralize this and anything else that needs to happen when we set or unset the blank page. ::: src/ephy-window.c @@ +1788,3 @@ + "ViewCombinedStopReload"); + ephy_action_change_sensitivity_flags (action, + SENS_FLAG_IS_BLANK, is_blank); Boy, the ephy_action_change_sensitivity_flags API is super confusing. So basically the actions won't be sensitive unless none of the flags are set? Is that what you want? Or am I reading the code wrong. @@ +2659,3 @@ G_CALLBACK (sync_tab_navigation), window, 0); + g_signal_connect_object (view, "notify::is-blank", Forgot to disconnect the handler?
Created attachment 205888 [details] [review] ephy-action-helper: document flags API I also lost a few minutes trying to remember how this thing worked, and actually remembered that last time I didn't document it when I figured out how it worked. So here's a docstring for it.
Created attachment 205891 [details] [review] ephy-web-view: remove uneeded is_blank call A trivial thing I found while working on this.
(In reply to comment #6) > Created an attachment (id=205891) [details] [review] > ephy-web-view: remove uneeded is_blank call > > A trivial thing I found while working on this. Just noticed I have a similar one in the main patch. Should I merge this into the main patch, or the other way around?
Created attachment 205892 [details] [review] ephy-web-view: remove uneeded is_blank calls Updated, with the other is_blank clean up from the main patch.
Created attachment 205893 [details] [review] Updated with your comments. Now using an internal _ephy_web_view_set_is_blank ephy-window: sync page actions with is-blank property Add ::is-blank property to EphyWebView and update EphyWindow to sync some of the page menu actions with it. There's no point in enabling save/reload/bookmark/etc on about:blank.
(In reply to comment #5) > Created an attachment (id=205888) [details] [review] > ephy-action-helper: document flags API > > I also lost a few minutes trying to remember how this thing worked, and > actually remembered that last time I didn't document it when I figured out how > it worked. > So here's a docstring for it. So, I understood what the function does correctly. My question stands: are you sure we want to use it the way we are using it?
Review of attachment 205888 [details] [review]: Done in bug #668510
Review of attachment 205892 [details] [review]: OK.
Review of attachment 205893 [details] [review]: ::: embed/ephy-web-view.c @@ +2514,3 @@ + + if (priv->is_blank != is_blank) + { Brace in the previous line. ::: src/ephy-window.c @@ +1788,3 @@ + "ViewCombinedStopReload"); + ephy_action_change_sensitivity_flags (action, + SENS_FLAG_IS_BLANK, is_blank); Most of the code is all the same. I'm going to say OK for now, but we should do some cleanup and just have an array with the action names to be disabled on blank. Then write this as a for loop. Really.
Done! Woohoo. Attachment 205892 [details] pushed as d626c6f - ephy-web-view: remove uneeded is_blank calls