GNOME Bugzilla – Bug 757228
[PATCHes] history-dialog.ui: Unbloat.
Last modified: 2018-08-03 20:40:49 UTC
Created attachment 314268 [details] [review] history-dialog.ui: Manually unbloat. Here are some random cleanings around the history dialog.
Created attachment 314269 [details] [review] EphyHistoryWindow: Use tree_selection.
Created attachment 314270 [details] [review] EphyHistoryWindow: Can copy only one url.
Created attachment 314271 [details] [review] history-dialog.ui: Kill a level of GtkBox.
Created attachment 314272 [details] [review] EphyHistoryWindow: Use liststore directly.
Created attachment 314273 [details] [review] EphyHistoryWindow: End-of-lines tabs.
Created attachment 314274 [details] [review] EphyHistoryWindow: Call response_cb from the UI file.
Created attachment 314275 [details] [review] history-dialog.ui: Use ToolButton. I don’t understand the current state of things… Am I missing something?
Created attachment 314276 [details] [review] history-dialog.ui: Use GtkGrid when possible.
Review of attachment 314268 [details] [review]: ::: src/resources/history-dialog.ui @@ +85,2 @@ <child internal-child="selection"> + <object class="GtkTreeSelection" id="tree_selection"> <!-- TODO use this --> What's with the new TODO?
Review of attachment 314269 [details] [review]: Question answered... well I don't see this as being much cleaner than the original code, but it's not worse either, so sure.
Review of attachment 314270 [details] [review]: Code is fine, but I had to play around a bit with the dialog to see why it's correct, so please leave a more descriptive commit message. We tend to follow https://wiki.gnome.org/Git/CommitMessages much more closely for Epiphany than you do for the games you maintain. ::: src/resources/history-dialog.ui @@ +227,3 @@ <property name="visible">True</property> <child> + <object class="GtkMenuItem"> Thanks for getting rid of the unnecessary ids. That's attention to detail.
Review of attachment 314271 [details] [review]: I presume this is correct; it's rather tough to review :p
Review of attachment 314272 [details] [review]: Again, I don't much see the value of such a change, but whatever. If one of the next patches is a commit that makes the dialog suck less, that would be great... I'm rather frustrated with how it reloads itself after every option, and how you can't tell that you've opened a page from history until you close the dialog and scroll to the end of the notebook.... Also, something in this patchset (or maybe in GTK+ master) broke the tooltips on the buttons on the bottom of the dialog. I'm trying to keep an eye out for that.
Review of attachment 314273 [details] [review]: ++
Review of attachment 314274 [details] [review]: ++
Review of attachment 314275 [details] [review]: My instinct is to say "yay nice cleanup" and mark this accepted, because I don't see anything wrong with it. But please check that this doesn't break the tooltips. I don't see why it would, though....
Review of attachment 314276 [details] [review]: Sigh... I dunno. I like GtkBox because it's simpler. But I know it's probably going to be removed, and we're going to have to do this eventually....
Review of attachment 314275 [details] [review]: Actually, this patch can't have broken the tooltips, because I applied the patches from this bug before you posted this one.
Yeah, the tooltips are broken with git master. Probably a GTK+ issue.
I didn’t pushed the two last patches for now, as I’ve spotted a difference of styling when using ToolButtons (and as I have some more cleanings comming).
Please do change the patch status when you push them, and add a link to the bug at the bottom of the commit message. 'git bz' does this all for you automatically! I've been meaning to make a git-bz package for Fedora, probably time to get around to that....
Created attachment 314353 [details] [review] Use GActions in the popup menu. Here are four patches for using GActions instead of callbacks. > Please do change the patch status when you push them Oh, yeah, forgot that. I’ll try to remember to do it.
Created attachment 314354 [details] [review] Split selection_changed callback in two functions.
Created attachment 314355 [details] [review] Split button_press callback in two functions.
Created attachment 314356 [details] [review] Use GAction for cleaning history.
Created attachment 314357 [details] [review] Create popup menu from a model. One more patch.
Created attachment 314358 [details] [review] Define keybindings in UI file. One more patch.
Created attachment 314365 [details] [review] Visual refresh (GtkActionBar, GtkSearchBar). Here is a visual refresh of the dialog, using a GtkSearchBar (always displayed) for placing the GtkSearchEntry (not taking all the width of the dialog anymore), killing the window’s borders according to the HIGs, replacing the old-fashioned GtkToolbar with a GtkActionBar (not animated; I could).
Created attachment 314366 [details] [review] Kill a (now) unneeded level of GtkBox. Nothing to see here, just an update made possible by the previous patch.
Created attachment 314368 [details] [review] Don’t unassign/reassign the model to the TreeView. I don’t understand this thing; can we remove it?
Note for the commiter: should add: update_selection_actions (self->private->action_group, FALSE); at the end of ephy_history_window_init(), at patch 314354. And rename “open-url” to “open-selection” and “open_url” to “open_selection” in all patches, as that’s what is done there.
Review of attachment 314353 [details] [review]: A bit unconventional, but I can see this becoming more common.
Review of attachment 314354 [details] [review]: OK, presuming the split is to allow you to call it elsewhere in a later patch?
Review of attachment 314355 [details] [review]: I guess this one is worth it regardless.
Review of attachment 314356 [details] [review]: OK
Review of attachment 314357 [details] [review]: Great. Do consider moving the menu to the top of the file, where it's easier to find.
Review of attachment 314358 [details] [review]: I understand that this patch doesn't change the previous behavior, but we really need to fix it. Why should opening a URL result in it being deleted from history? I noticed this bug a while back and I think we need to fix it.
Review of attachment 314365 [details] [review]: OK
Review of attachment 314366 [details] [review]: OK
Review of attachment 314368 [details] [review]: THANKYOU!
Review of attachment 314368 [details] [review]: Actually, this last commit broke everything. Now Epiphany hangs forever if you use either of the buttons.
Created attachment 314570 [details] [review] EphyHistoryWindow: Remove unused function.
Review of attachment 314570 [details] [review]: ++
Comment on attachment 314365 [details] [review] Visual refresh (GtkActionBar, GtkSearchBar). Any plans to return to this?
Comment on attachment 314365 [details] [review] Visual refresh (GtkActionBar, GtkSearchBar). It's old and will surely need rebased now.
Comment on attachment 314366 [details] [review] Kill a (now) unneeded level of GtkBox. Ditto.
Arnaud, we are so close to getting all your changes in. Any chance you can put the finishing touch on the remaining patches, please?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/281.