GNOME Bugzilla – Bug 681089
Add search bar
Last modified: 2016-03-31 14:00:10 UTC
The searchbar is based on current gnome-documents behaviour and style, as well as the same GtkWidget code shared in libgd. It will search as you type and filter the boxes. You can activate the result if there is only a single match currently (I am not sure that's what the design says). Anyway, it's hopefully a good and useful first iteration.
Created attachment 220171 [details] [review] Removing debug message
Created attachment 220172 [details] [review] revealer: learn to resize actor allocation too
Created attachment 220173 [details] [review] revealer: add read/write revealed property
Created attachment 220174 [details] [review] libgd: import some widgets from gnome-documents These files should be kept in sync with upstream, if possible. That's why I also left the trailing white spaces.
Created attachment 220175 [details] [review] libgd: generate gd-1.0.vapi binding
Created attachment 220176 [details] [review] Insert items in collection not in the view The view might show a filtered or different version.
Created attachment 220177 [details] [review] collection-view: use a filtered model Learn to filter the view by using the model_visible () callback, using the app.filter Boxes.CollectionFilter to contain the current filtering criterias.
Created attachment 220178 [details] [review] collection-view: add an activate method When then search entry is pressed, we want to activate whatever item is matching the search, to open it (start and show display).
Created attachment 220179 [details] [review] topbar: simplify and generalize a function Rename update_select_btn_sensitivity () to update_select_btn () since it could perform and other UI changes, and we want name consitency with other similar UI update functions.
Created attachment 220180 [details] [review] topbar: add a search button The button is active whenever the search bar is visible
Created attachment 220181 [details] [review] search: add a search bar The searchbar is based on current gnome-documents behaviour and style, as well as the same GtkWidget code shared in libgd. It will search as you type and filter the boxes. You can activate the result if there is only a single match currently (I am not sure that's what the design says). Anyway, it's hopefully a good and useful first iteration.
(In reply to comment #11) > Created an attachment (id=220181) [details] [review] > search: add a search bar > > The searchbar is based on current gnome-documents behaviour and style, > as well as the same GtkWidget code shared in libgd. Free cookies if the search bar is consistent with other applications. Currently it's a mess in GNOME. (bug 672394, bug 661564)
(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=220181) [details] [review] [details] [review] > > search: add a search bar > > > > The searchbar is based on current gnome-documents behaviour and style, > > as well as the same GtkWidget code shared in libgd. > > Free cookies if the search bar is consistent with other applications. > Currently it's a mess in GNOME. (bug 672394, bug 661564) I used the information I gathered during GUADEC, but I think we should not hold this feature until the design is "finalizes". We can fix the remaining style/behaviour issues in the following releases.
Review of attachment 220171 [details] [review]: ACK
Review of attachment 220172 [details] [review]: I might be able to give a verdict here if you could add a bit more detail in the log: Whats the current behaviour and why this change is needed.
Review of attachment 220172 [details] [review]: the patch doesn't change behaviour unless resize is set to true, if true then we change the actor allocation before/during animation, so that it takes 0 space when unrevealed, and its max(children) when revealed. This allows to put the search bar between 2 actors and actually "grow"/"shrink" the allocation between them when revealed/unrevealed
Review of attachment 220173 [details] [review]: So if i understood correct, 'revealed' is target state and '_visible' is current state? Why do we need to differentiate? ::: src/revealer.vala @@ +4,3 @@ public class Boxes.Revealer: Clutter.Actor { private bool horizontal; + private bool _visible; i thought we don't do '_' prefixing for private fields except for place-holders for props (like _revealed below).
Review of attachment 220174 [details] [review]: Hm.. I understood in GNOME OS BoF that Cosimo will make this availble as a module for apps to use as submodule. Have you asked cosimo if he is gonna do that soon?
Review of attachment 220175 [details] [review]: Looks good. ACK if/when the dependent patch is ACK'ed.
Review of attachment 220176 [details] [review]: Seems right.
(In reply to comment #17) > Review of attachment 220173 [details] [review]: > > So if i understood correct, 'revealed' is target state and '_visible' is > current state? Why do we need to differentiate? > > ::: src/revealer.vala > @@ +4,3 @@ > public class Boxes.Revealer: Clutter.Actor { > private bool horizontal; > + private bool _visible; > > i thought we don't do '_' prefixing for private fields except for place-holders > for props (like _revealed below). There is a parent visible property clashing. I suggest we rename this _visible in children_visible instead.
(In reply to comment #18) > Review of attachment 220174 [details] [review]: > > Hm.. I understood in GNOME OS BoF that Cosimo will make this availble as a > module for apps to use as submodule. Have you asked cosimo if he is gonna do > that soon? I actually discussed that several time with Cosimo, and I guess he might even be waiting for me to do it. But not just yet, because libgd is actually a bunch of things that we need to clean up first, and that was not my goal here.
Created attachment 220733 [details] [review] revealer: add read/write revealed property
(In reply to comment #23) > (In reply to comment #18) > > Review of attachment 220174 [details] [review] [details]: > > > > Hm.. I understood in GNOME OS BoF that Cosimo will make this availble as a > > module for apps to use as submodule. Have you asked cosimo if he is gonna do > > that soon? > > I actually discussed that several time with Cosimo, and I guess he might even > be waiting for me to do it. But not just yet, because libgd is actually a bunch > of things that we need to clean up first, and that was not my goal here. Yup, he was hoping for you to do it ;)
Review of attachment 220177 [details] [review]: Looks good otherwise. ::: src/collection-view.vala @@ -237,0 +241,13 @@ + + private CollectionItem get_item_for_path (Gtk.TreePath path) { + Gtk.TreeIter filter_iter, iter; ... 10 more ... I'd keep this code agnostic of filtering itself and have a function like this in CollectionFilter to call from here: public bool item_qualifies (CollectionItem item);
Review of attachment 220178 [details] [review]: * In the log * 'when then' -> 'when'. * 'entry is pressed' -> 'entry is activated' ? * You are not using this method while the log suggests that you do?
Review of attachment 220179 [details] [review]: * Log * 'simplify and generalize a function' -> 'simplify & generalize a function name' * 'could perform and' -> 'could perform' ? * consitency -> consistency Otherwise good. Please remember to fix the commit log before pushing this time. :)
Review of attachment 220180 [details] [review]: * Log * active -> activated ? * Button appears joined to the selection button. Adding some padding/space in btw? Looks good otherwise.
Review of attachment 220181 [details] [review]: I'm assuming that this patch apears after the 'search button' patch only in bz? If not, you want to swap the order of these patches. "Anyway, it's hopefully a good and useful first iteration." actually, iIts an awesome first iteration. :) ::: src/searchbar.vala @@ +7,3 @@ + + private uint refilter_delay_id; + static const uint refilter_delay = 200; // in ms 10 points for commenting about units but 'm' could mean 'milli' or 'micro' so better specificy 'milliseconds'. @@ +31,3 @@ + App.app.view.refilter (); + refilter_delay_id = 0; + return false; coding-style nitpick: return on new line. @@ +57,3 @@ + + public void grab_focus () { + Gd.entry_focus_hack (entry, Gtk.get_current_event_device ()); nitpick: indented twice as usual.. @@ +121,3 @@ + entry = new Gd.TaggedEntry (); + entry.width_request = 260; + // entry.no_show_all = true; commented out code without any explanation of why its desired and why its commented out is never good IMHO.
Review of attachment 220172 [details] [review]: ok, ACK!
Review of attachment 220174 [details] [review]: Fair enough.
Review of attachment 220181 [details] [review]: ::: src/searchbar.vala @@ +7,3 @@ + + private uint refilter_delay_id; + static const uint refilter_delay = 200; // in ms m is milli, u or µ is micro, no ambiguity there.
Review of attachment 220733 [details] [review]: Renaming also answered my other question about this patch. Cheers for good names.
Review of attachment 220181 [details] [review]: ::: src/searchbar.vala @@ +7,3 @@ + + private uint refilter_delay_id; + static const uint refilter_delay = 200; // in ms If Christophe thinks there is no abmiguity, there probably isn't. :)
Review of attachment 220177 [details] [review]: ::: src/collection-view.vala @@ -237,0 +241,13 @@ + + private CollectionItem get_item_for_path (Gtk.TreePath path) { + Gtk.TreeIter filter_iter, iter; ... 10 more ... done
Review of attachment 220178 [details] [review]: done
Review of attachment 220180 [details] [review]: technically the propert is named "active", but activated is perhaps better in the log. The styling/spacing is fixed with the usage of Gd.MainToolbar (not part of this bug)
Review of attachment 220181 [details] [review]: ::: src/searchbar.vala @@ +31,3 @@ + App.app.view.refilter (); + refilter_delay_id = 0; + return false; done @@ +57,3 @@ + + public void grab_focus () { + Gd.entry_focus_hack (entry, Gtk.get_current_event_device ()); done @@ +121,3 @@ + entry = new Gd.TaggedEntry (); + entry.width_request = 260; + // entry.no_show_all = true; removed
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
diff --git a/libgd/Makefile.am b/libgd/Makefile.am index 84f978d..a5e8fda 100644 --- a/libgd/Makefile.am +++ b/libgd/Makefile.am @@ -71,11 +71,12 @@ VAPIS = gd-1.0.vapi BUILT_SOURCES = $(VAPIS) gd-1.0.vapi: $(builddir)/Gd-1.0.gir - $(AM_V_GEN)$(VAPIGEN) \ - --library gd-1.0 \ - --pkg gio-2.0 \ - --pkg gobject-2.0 \ - --pkg gtk+-3.0 \ + $(AM_V_GEN)$(VAPIGEN) \ + --vapidir=$(top_srcdir)/vapi/upstream \ + --library gd-1.0 \ + --pkg gio-2.0 \ + --pkg gobject-2.0 \ + --pkg gtk+-3.0 \ $< EXTRA_DIST = $(VAPIS) is needed to fix compilation with vala 0.16
And I sometimes get build failures on fresh git clones with parallel make, make complains about libcommon.h not existing, I think something like this is needed: diff --git a/src/Makefile.am b/src/Makefile.am index 99a1008..d3011ab 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -56,7 +56,7 @@ libcommon_a_SOURCES = \ libcommon_a_CFLAGS = $(BOXES_COMMON_CFLAGS) -libcommon_vala.stamp common.vapi: $(libcommon_a_VALASOURCES) +libcommon_vala.stamp common.vapi libcommon.h: $(libcommon_a_VALASOURCES) $(AM_V_VALAC)$(VALAC) $(VALAFLAGS) $(VALA_DEBUG_FLAGS) \ --enable-experimental \ --vapidir=$(srcdir)/ \
ack, feel free to commit build-sys fixes.