GNOME Bugzilla – Bug 760164
Use GtkFlowBox
Last modified: 2016-08-22 14:41:59 UTC
There are various reports regarding performance issues in music (745651 and 759886), for example). I have it on good authority (Michael and Felipe!) that some of this is due to GtkIconView inefficiency, and that porting to GtkFlowBox would be a good step forward. This will require implementing selection mode for GtkFlowBox. Some relevant work can be found in gnome-todo: https://git.gnome.org/browse/gnome-todo/tree/src/gtd-task-list-item.c
Created attachment 333015 [details] [review] view: factor out function to setup the view Since the Albums view will handle the internal view differently, it makes sense to add a new method that will be overriden in the future. This patch moves the code related to setting up the view into a separate function.
Created attachment 333016 [details] [review] view: initialize main box before setup_view While initializing the Albums view, we will need to access the main box, which must be initialized when setup_view() gets called. This patch moves the code around so the box is initialized when setup_view() is called.
Created attachment 333017 [details] [review] data: add AlbumCover widget Now that we're moving away from a cellrenderer-based rendering, all visible elements are Gtk widgets, forcing us to customly manage the albums. This commit introduces the AlbumCover UI definition, which will be consumend in the future when creating albums for the flowbox.
Created attachment 333018 [details] [review] albums: use a GtkFlowBox internally After preparing ground, the move is finally done: the Albums view moves away from GdMainView to GtkFlowBox. This patch makes the Albumns class override some methods to put the flowbox in place rather than the main view, and is already able to display the albums and interact with them. Appearently, it runs incredibly smoother :)
Created attachment 333019 [details] [review] view: add 'selection-mode' property By adding a new property, we'll be able to skip manual management of selection mode checkboxes and simply bind them.
Created attachment 333020 [details] [review] toolbar: remove exquisite code Toolbar is managing the view's selection mode signal handling. This is a very bad idea, and makes it impossible to move to the flowbox. Fix this by properly letting the views manage their own business.
Created attachment 333021 [details] [review] albums: minor tweaks on selection mode
Created attachment 333022 [details] [review] albums: readd selection mode The previous selection mode code wouldn't work as we move away from a model-based approach. Fix that by handling selection manually.
Created attachment 333023 [details] [review] albums: fix select/unselect all
Created attachment 333024 [details] [review] views: unselect all when quitting selection mode
Created attachment 333025 [details] [review] albums: show a loading icon while fetching album art
Created attachment 333026 [details] [review] albums: allow up to 10 albums per row More than that and it'll be cluttered.
Created attachment 333027 [details] [review] albums: bring back selection mode on right click We used to have this for free when using GdMainView but, as long as we're not using it anymore, we have to deal with it manually. This patch reimplements the selection mode on Albums view with right-click.
Created attachment 333033 [details] [review] albums: fix select/unselect all Fix wrong call.
Review of attachment 333033 [details] [review]: ::: gnomemusic/view.py @@ +283,3 @@ + count += self._set_selection(value, _iter) + if self.model[_iter][5]: + self.model.set(_iter, [6], [value]) this can be written as 'self.model[_iter][6] = value' now.
Review of attachment 333018 [details] [review]: ::: gnomemusic/view.py @@ +417,3 @@ + elif remaining == 0: + self.window.notification.dismiss() + self.view.show() indent too far @@ +438,3 @@ + child.show() + + self.cache.lookup(item, self._iconWidth, self._iconHeight, self._on_lookup_ready, indent after 3rd argument for pep-8 linelength
Looks like awesome work overall, haven't had time to review the patchset as a whole yet. But I can see it works like a charm. A few things I encountered. * code style follow pep-8 (it does so fine mostly, just a few nitpicks here and there) * please add docstrings for public class functions at least, see the readme. For private functions if it adds clarity * right-click selectionmode needs a fix, just try rightclicking a few times and you will see why. (probably trivial) * why not use the flowbox selection mode? Was this a design decision (to keep the looks as it was)? We should probably discuss with the designers what is best, but I prefer to use the gtk provided mode if it's there. If we keep the current way, it probably needs styling.
Created attachment 333084 [details] [review] albums: fix select/unselect all Add docstring for public functions.
Created attachment 333085 [details] [review] albums: optimize select/unselect all Iterating over all the albums and changing the ::active property would send hundreds of notify::active signals, which in turn makes it go in and out of Python to C to Python in a way that makes the UI freeze for a second. Fix that by blocking all these signals and updating the list of selected items manually.
(In reply to Marinus Schraal from comment #17) > Looks like awesome work overall, haven't had time to review the patchset as > a whole yet. But I can see it works like a charm. Awsome, thanks. > * code style follow pep-8 (it does so fine mostly, just a few nitpicks here > and there) Fixed. > * please add docstrings for public class functions at least, see the readme. > For private functions if it adds clarity Done. > * right-click selectionmode needs a fix, just try rightclicking a few times > and you will see why. (probably trivial) Fixed. > * why not use the flowbox selection mode? Was this a design decision (to > keep the looks as it was)? We should probably discuss with the designers > what is best, but I prefer to use the gtk provided mode if it's there. If we > keep the current way, it probably needs styling. The GtkFlowBox selection mode looks quite different from any other GNOME content app's selection mode. I had direct instructions from Lapo that we should start using checkbuttons with 'osd' styleclass rather than that big bogus checkbutton (which Adwaita doesn't seem to support correctly nowadays).
Created attachment 333252 [details] [review] albums: optimize select/unselect all Fixed the unblocking of signals, sorry about that
Created attachment 333266 [details] [review] albums: Fix right-click selection mode Only set the child check active when entering actual selection mode. Let _on_selection_mode_request handle showing/hiding of the checkmarks.
I squashed the commits a bit and added some minor fixups, then pushed it. Now lets get it some testing. Thanks for the awesome work, this is a big improvement. This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.