GNOME Bugzilla – Bug 783290
Setting the custom home page field to "about:blank" prevents subsequent changes
Last modified: 2017-07-13 04:20:39 UTC
To reproduce: * Under Preferences -> Homepage select Custom, and then enter about:blank The radio immediately switches to "Blank page", making the text field insensitive. The Custom radio can no longer be selected, and prefs must be closed then opened to escape the UI bug. A quick fix would probably be to clear the "Custom" text field as soon as another radio option is selected, since custom contents of the field aren't saved to settings when "Custom" is not selected anyway. With the above fix, when "about:blank" is entered, "Blank page" would be immediately selected and the field would clear, allowing the user to subsequently re-select Custom immediately.
This seems like a good easy bug for newcomers to try fixing. The place to look is src/prefs-dialog.c.
I'll try working on this (looks like a simple first bug).
Yeah, it's probably something simple. Add some printf statements to the code until you figure out where it's going wrong, and ask here or in #epiphany if you have questions. Good luck!
Is it possible to set up the build environment with builder and flatpak instead of jhbuild? It seems that there are some libraries used by epiphany that aren't in the gnome flatpak runtime - maybe those can be manually added in? That would make setup (I'm using a Debian system with old libraries) much easier.
Right now I can only support JHBuild. Here are instructions: https://wiki.gnome.org/Apps/Web/Development That said, making sure the Flatpak build works is high on my TODO list. There is a flatpak-builder manifest here: https://git.gnome.org/browse/gnome-apps-nightly/tree/org.gnome.Epiphany.json That might work for you. But I have no idea if it is up-to-date and actually works or not.
Created attachment 355116 [details] video of the problem
It appears that the bug (at least as seen on the latest code from git) is a bit different from what was reported. Looking at the attached video, when I type in about:blank the radio switches over to "blank page" automatically. However, I can still select the custom radio fine. The issue (which is different from reported) is that I can only enter a single character into the box before the field becomes insensitive. In the case of the video, I entered "e" and the program would not let me insert anything else. I'll try to work on fixing this.
The behaviour I see on 3.24.x is as I described, so must have either changed since on Git master or there's another factor involved :) I'm using GNOME Wayland if that makes any difference.
Created attachment 355132 [details] [review] prefs-dialog patch #1 This patch causes the GTKEntry containing the custom homepage address to clear itself if a different radio button (i.e. the "new tab page" or "blank page" button is selected). It's a bit awkward, since the clearing of the GTKEntry causes the program to automatically switch over to the "new tab page" button, and for the blank page case I had to restore about:blank manually into Gsettings. I've done some basic testing of the different cases and it appears to work properly.
Created attachment 355197 [details] [review] prefs-dialog patch # 2 This is a more elegant version of the previous patch, and does the same task of automatically clearing the custom homepage field without manipulating the radio buttons in the code. The "set mapping" functions for new tab and blank page now clear the text entry when they are called. The callback function activated when the entry has been changed has been altered to only activate when the custom homepage radio button is checked, so that the clearing of the text entry mentioned above does not also blank out the gsettings for the homepage url.
Review of attachment 355197 [details] [review]: Thanks for your patch! It looks good. Certainly better than your first attempt. ;) I just have a few code style comments. If you're interested in related bugs, bug #783299 and bug #783300 both look easy. ::: src/prefs-dialog.c @@ +1333,3 @@ gpointer user_data) { + PrefsDialog *dialog = user_data; In most code, I prefer to use GObject dynamic (runtime type checking) casts: PrefsDialog *dialog = PREFS_DIALOG (user_data); But in this case, ignore that, because it's so much cleaner to just change the function signature of the callback, and I think the benefits of doing that outweigh the loss of type checking. Just change the final parameter from "gpointer user_data" to "PrefsDialog *dialog". This works because signal callbacks are invoked via foreign function interface. @@ +1337,3 @@ if (!g_value_get_boolean (value)) return NULL; + gtk_entry_set_text (GTK_ENTRY (dialog->custom_homepage_entry), ""); I prefer a code style that uses more blank lines in general. I would leave one blank line above and below your new one. @@ +1360,3 @@ gpointer user_data) { + PrefsDialog *dialog = user_data; Change the function signature instead. @@ +1364,3 @@ if (!g_value_get_boolean (value)) return NULL; + gtk_entry_set_text (GTK_ENTRY (dialog->custom_homepage_entry), ""); Leave a blank line. @@ +1391,3 @@ if (!g_value_get_boolean (value)) { gtk_widget_set_sensitive (dialog->custom_homepage_entry, FALSE); + gtk_entry_set_text (GTK_ENTRY (dialog->custom_homepage_entry), ""); Careful with the indentation: you accidentally used a tab character here instead of two spaces. Your editor has betrayed you! @@ +1410,3 @@ { + if (gtk_toggle_button_get_active + (GTK_TOGGLE_BUTTON (dialog->custom_homepage_radiobutton))) Don't break this between two lines, that makes it harder to read. It's not too long to fit on one line. We don't have an 80 character limit.
Created attachment 355232 [details] video of special case Special case where the custom dialog breaks if I highlight the existing text fully and attempt to type new text on top of it. This is due to the computer treating the action of typing new text on top of highlighted text as two seperate actions, delete text (which triggers the "new text" button since the entry is now empty) and then adding the new text.
Created attachment 355233 [details] [review] prefs-dialog patch # 3 I have made the stylistic changes mentioned in the review, and also added additional code to correct for the bug shown in the new video. Now, if there is still text in the entry and the "new homepage" button is pressed down, the text entry will remain active (and therefore the custom homepage button will reactivate once the new character appears)... this process happens so quickly you don't notice the new homepage button activating and deactivating.
Review of attachment 355233 [details] [review]: OK great, almost there! Next step: please submit a git formatted patch so that includes a real commit message! You can use 'git format-patch' or gitg: https://wiki.gnome.org/Newcomers/SubmitPatch ::: src/prefs-dialog.c @@ +1331,3 @@ new_tab_homepage_set_mapping (const GValue *value, const GVariantType *expected_type, + PrefsDialog *dialog) Ah, whoops, since these are settings bindings and not signal callbacks, the type actually is important: did you see the compiler warnings this introduced? You'll have to fix them: PrefsDialog *dialog = PREFS_DIALOG (user_data); @@ +1337,3 @@ + + gtk_entry_set_text (GTK_ENTRY (dialog->custom_homepage_entry), ""); + gtk_widget_set_sensitive (dialog->custom_homepage_entry, FALSE); OK, interesting bug you've discovered. I think this is the perfect solution for fixing it. But it looks strange to a developer reading this code. You should add a *brief* warning comment so that somebody doesn't remove this code in the future. You don't need to explain the bug in detail, just leave a brief warning. @@ +1379,3 @@ if (setting && setting[0] != '\0' && g_strcmp0 (setting, "about:blank") != 0) g_value_set_boolean (value, TRUE); + Style nit: don't add the blank line here, it's not helping to organize the code. @@ +1415,3 @@ + g_settings_set_string (EPHY_SETTINGS_MAIN, EPHY_PREFS_HOMEPAGE_URL, + gtk_entry_get_text (entry)); + else if ((gtk_entry_get_text (entry) != NULL) && gtk_toggle_button_get_active Style nit: since this second condition is long and requires braces, add them to the first condition too, for parallelism. Also, either move the line break to right after the &&, or get rid of it entirely. Don't put it at the start of a function call. @@ +1420,3 @@ + gtk_entry_get_text (entry)); + gtk_widget_set_sensitive (dialog->custom_homepage_entry, TRUE); + gtk_widget_grab_focus (dialog->custom_homepage_entry); Can you explain why you need the gtk_widget_set_sensitive() and gtk_widget_grab_focus() here? I doubt they're really needed.
PrefsDialog *dialog = PREFS_DIALOG (user_data); caused the following compiler warning: [1/16] Compiling C object 'src/ephymain@sha/prefs-dialog.c.o'. ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c: In function ‘new_tab_homepage_set_mapping’: ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1335:3: warning: implicit declaration of function ‘PREFS_DIALOG’ [-Wimplicit-function-declaration] PrefsDialog *dialog = PREFS_DIALOG (user_data); ^ ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1335:25: warning: initialization makes pointer from integer without a cast PrefsDialog *dialog = PREFS_DIALOG (user_data); ^ ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c: In function ‘blank_homepage_set_mapping’: ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1366:25: warning: initialization makes pointer from integer without a cast PrefsDialog *dialog = PREFS_DIALOG (user_data); ^ Using the "EPHY_PREFS_DIALOG" cast (i.e. PrefsDialog *dialog = EPHY_PREFS_DIALOG (user_data)) created no warnings. That cast was used in other parts of the code (of which I didn't write), and I guess that's the right one to use... although I don't know why. gtk_widget_set_sensitive() and gtk_widget_grab_focus() are needed because they ensure that the text field remains active if I select all the text and start typing (the special edge I mentioned earlier). When the text is deleted (the first step), the "new tab" button is triggered automatically and therefore custom_homepage_set_mapping causes the text entry to become unsensitive. custom_homepage_set_mapping doesn't get called again until I start typing more stuff in the text field, which I can't do since the field is unsensitive.
Created attachment 355472 [details] [review] prefs-dialog patch # 4 and heres the actual patch
(In reply to specter92 from comment #15) > PrefsDialog *dialog = PREFS_DIALOG (user_data); > > caused the following compiler warning: > > > [1/16] Compiling C object 'src/ephymain@sha/prefs-dialog.c.o'. > ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c: In function > ‘new_tab_homepage_set_mapping’: > ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1335:3: warning: > implicit declaration of function ‘PREFS_DIALOG’ > [-Wimplicit-function-declaration] > PrefsDialog *dialog = PREFS_DIALOG (user_data); > ^ > ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1335:25: warning: > initialization makes pointer from integer without a cast > PrefsDialog *dialog = PREFS_DIALOG (user_data); > ^ > ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c: In function > ‘blank_homepage_set_mapping’: > ../../../../jhbuild/checkout/epiphany/src/prefs-dialog.c:1366:25: warning: > initialization makes pointer from integer without a cast > PrefsDialog *dialog = PREFS_DIALOG (user_data); > ^ > > Using the "EPHY_PREFS_DIALOG" cast (i.e. PrefsDialog *dialog = > EPHY_PREFS_DIALOG (user_data)) created no warnings. That cast was used in > other parts of the code (of which I didn't write), and I guess that's the > right one to use... although I don't know why. Yup, I just guessed the name of the cast wrong! It should be defined at the top of prefs-dialog.h so you can look at the definition there. > gtk_widget_set_sensitive() and gtk_widget_grab_focus() are needed because > they ensure that the text field remains active if I select all the text and > start typing (the special edge I mentioned earlier). When the text is > deleted (the first step), the "new tab" button is triggered automatically > and therefore custom_homepage_set_mapping causes the text entry to become > unsensitive. custom_homepage_set_mapping doesn't get called again until I > start typing more stuff in the text field, which I can't do since the field > is unsensitive. OK!
Review of attachment 355472 [details] [review]: OK great. ::: src/prefs-dialog.c @@ +1420,3 @@ + gtk_entry_get_text (entry)); + } + else if ((gtk_entry_get_text (entry) != NULL) && I'm going to make one style change before pushing this. The else if should be up on the same line as the closing brace.
Created attachment 355474 [details] [review] final patch That was a quick reply... I was about to turn off my computer!
(In reply to Michael Catanzaro from comment #18) > I'm going to make one style change before pushing this. The else if should > be up on the same line as the closing brace. I actually already made that change and pushed it. Thanks! It'll be in 3.24.3.
And I just noticed you adjusted some of the spacing for parameter names, so a follow-up commit: https://git.gnome.org/browse/epiphany/commit/?id=1db0ea863a42aa4e2f6591d6badfb4fea15385e0 The names get aligned with themselves, leaving the asterisks off to the side. I know GNOME style is a little different than what you're probably used to... you'll get used to it if you keep contributing.