After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768275 - search queries instant apply
search queries instant apply
Status: RESOLVED FIXED
Product: dconf-editor
Classification: Other
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: dconf-editor maintainer(s)
dconf-editor maintainer(s)
: 770172 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-07-01 11:59 UTC by Jakub Steiner
Modified: 2017-11-28 06:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Cleaning patchset. (18.09 KB, application/x-7z-compressed)
2017-11-17 23:08 UTC, Arnaud B.
  Details
Non-rebased, wip patch. (36.08 KB, patch)
2017-11-17 23:15 UTC, Arnaud B.
none Details | Review
Davi's wip patch (23.38 KB, patch)
2017-11-18 03:42 UTC, Davi
none Details | Review
Davi's new wip patchset (11.53 KB, application/zip)
2017-11-22 03:27 UTC, Davi
  Details
Cleaning patchset. (17.49 KB, application/x-7z-compressed)
2017-11-23 15:02 UTC, Arnaud B.
  Details
Davi's wip search patch (42.98 KB, patch)
2017-11-24 03:39 UTC, Davi
none Details | Review
Davi's wip search patch (10.93 KB, patch)
2017-11-26 03:17 UTC, Davi
none Details | Review
Davi's wip search patch (11.02 KB, patch)
2017-11-26 07:39 UTC, Davi
none Details | Review
Fix miscasting to RegistryView (1.17 KB, patch)
2017-11-27 00:26 UTC, Davi
none Details | Review

Description Jakub Steiner 2016-07-01 11:59:57 UTC
Good job with the redesign. One thing that I noticed is that each queries didn't behave the same as other gnome apps and required hitting return. Would be nice to make it more predictable and consistent and trigger the search (with an idle timer to allow uninterrupted typing).
Comment 1 Arnaud B. 2016-08-21 10:09:08 UTC
*** Bug 770172 has been marked as a duplicate of this bug. ***
Comment 2 Arnaud B. 2017-11-17 23:08:07 UTC
Created attachment 363953 [details]
Cleaning patchset.

The search is the last part of the code to use the old treeview-based functions. Changing the search is removing them, allowing a big cleaning. Here is a preparation patchset, that changes many things. Warning, it’s a manual rebase (on an intrusive patchset), there might be some mistakes (but looks like it’s compiling without error and working back).
Comment 3 Arnaud B. 2017-11-17 23:15:02 UTC
Created attachment 363954 [details] [review]
Non-rebased, wip patch.

Here is my non-rebased, wip patch for search. If it could help.

I’d like the searchbar to move as a DConfWindow child. And the search to apply for now on a ListBox in another child of the main stack. For future things.

For the search itself, I’d like to have multiple “search engines”:
 * a “Local search” that searches in the current folder (so that when you trigger the search bar, the search is empty but it shows the same content as before, with the header; and then, you can filter local thing like the old “type to move to row” functions);
 * a “Folder search”, designed to be fast;
 * a “Key search”, displayed at the end of the listbox (so it has the time to load, looking in keys descriptions).

Basically, I’d like the search to be mainly independent of the current tree created, as it is planned to be removed (let’s discover content in each click, that would help for bug https://bugzilla.gnome.org/show_bug.cgi?id=731750 also).
Comment 4 Davi 2017-11-18 03:19:49 UTC
Your wip patch does not apply on top of the cleaning patchset. Should I do something else?
Comment 5 Davi 2017-11-18 03:42:06 UTC
(In reply to Arnaud B. from comment #3)
> For the search itself, I’d like to have multiple “search engines”:
>  * a “Local search” that searches in the current folder (so that when you
> trigger the search bar, the search is empty but it shows the same content as
> before, with the header; and then, you can filter local thing like the old
> “type to move to row” functions);
>  * a “Folder search”, designed to be fast;
>  * a “Key search”, displayed at the end of the listbox (so it has the time
> to load, looking in keys descriptions).

Do you mean to have three alternative search modes? Or are those some kind of ranking rules for combining everything in the same view? I was trying to achieve the latter, but did not get to write the code yet. I'll attach my sketched patch for reference, but it's still very preliminary: the ranking is only based on location (proximity to current path), so an exact match might be ranked low because it is far away; also, the keys are searched depth-first from the root, so even if the current location is ranked higher, it might be searched last, making it take long to appear. There are likely other issues as well.
 
> Basically, I’d like the search to be mainly independent of the current tree
> created, as it is planned to be removed (let’s discover content in each
> click, that would help for bug
> https://bugzilla.gnome.org/show_bug.cgi?id=731750 also).

If I understand correctly, the idea is to rewrite most of the dconf-model.vala so that it does not cache the settings tree and re-fetches content on every path change?
Comment 6 Davi 2017-11-18 03:42:56 UTC
Created attachment 363962 [details] [review]
Davi's wip patch
Comment 7 Arnaud B. 2017-11-18 07:30:13 UTC
(In reply to Davi from comment #4)
> Your wip patch does not apply on top of the cleaning patchset. Should I do
> something else?

It’s just for reference on where I put widgets. I’ll maybe update/clean it when I have time, to compare with your approach, but that’s not really important.

(In reply to Davi from comment #5)
> I'll attach my sketched patch for reference, but it's still very preliminary:

Honestly, that’s not so bad already. :)

> Do you mean to have three alternative search modes? Or are those some kind
> of ranking rules for combining everything in the same view? I was trying to
> achieve the latter, but did not get to write the code yet.

I’d like all combined in the same view, as you’ve done. But, the current folder should be searched *fast*, so a specific search request might be done for it; and, I think that before displaying keys that (more or less) match, there should be a specific header under which *folders* which names matches are displayed (their complete path could appear in the row), and that’s a specific search engine.

> the ranking is only based on location (proximity to current path), so an
> exact match might be ranked low because it is far away;

The order of the keys result is not so important in my mind. I may be proven wrong, and every thing made better is an improvement (!), but I won’t stress with the ordering.

> If I understand correctly, the idea is to rewrite most of the
> dconf-model.vala so that it does not cache the settings tree and re-fetches
> content on every path change?

Yeps. There might be caching involved, but the whole tree won’t be created at application startup. Because that’s rarely useful, and because the tree should be flexible anyway (think of “the hydra”: relocatable schemas). The work in the patchset already removes any parent reference from SettingObject, it’s mainly a story of rewriting the SettingsModel class after that.
Comment 8 Arnaud B. 2017-11-18 07:47:40 UTC
There could even be a Bookmarks search engine, which results are displayed just after the Current Path results…
Comment 9 Davi 2017-11-18 21:18:42 UTC
(In reply to Arnaud B. from comment #7)
> Yeps. There might be caching involved, but the whole tree won’t be created
> at application startup. Because that’s rarely useful, and because the tree
> should be flexible anyway (think of “the hydra”: relocatable schemas). The
> work in the patchset already removes any parent reference from
> SettingObject, it’s mainly a story of rewriting the SettingsModel class
> after that.

I think it is a very interesting move, making the model more dynamic. But from what I can understand, that won't help much with relocatable schemas. I could not find a way to relate a relocatable schema with a given path. The schema does not report any paths with g_settings_schema_get_path and keys have no path-related properties/methods. DConf is used to retrieve such keys from a parent path right now, but dconf knows nothing of schemas. Am I missing something? Seems like current GSettings API make relocatable schemas impossible to discover reliably.
Comment 10 Arnaud B. 2017-11-20 15:58:43 UTC
(In reply to Davi from comment #9)
> I think it is a very interesting move, making the model more dynamic. But
> from what I can understand, that won't help much with relocatable schemas. I
> could not find a way to relate a relocatable schema with a given path. The
> schema does not report any paths with g_settings_schema_get_path and keys
> have no path-related properties/methods. DConf is used to retrieve such keys
> from a parent path right now, but dconf knows nothing of schemas. Am I
> missing something? Seems like current GSettings API make relocatable schemas
> impossible to discover reliably.

You’re not missing anything, it’s impossible to do. But users do not care about that, and that’s why there are three related bugs[1][2][3] not counting duplicates, API propositions, and others complaints. That’s why a highly dynamic view is needed, so that in the future random schemas could be applied wherever it’s needed. Yay. :)

[1] https://bugzilla.gnome.org/show_bug.cgi?id=762803 about the warning
[2] https://bugzilla.gnome.org/show_bug.cgi?id=755925 for global bug
[3] https://bugzilla.gnome.org/show_bug.cgi?id=649408 about orphans keys
Comment 11 Davi 2017-11-20 18:49:20 UTC
(In reply to Arnaud B. from comment #10)
> You’re not missing anything, it’s impossible to do. But users do not care
> about that, and that’s why there are three related bugs[1][2][3] not
> counting duplicates, API propositions, and others complaints. That’s why a
> highly dynamic view is needed, so that in the future random schemas could be
> applied wherever it’s needed. Yay. :)

I've bitten the bullet. I've written a tentative of making the model dynamic. And also a second patch with a proposal for a way to allow users to manually define where the relocatable patches should be used. As the justification for the spartan GSettings API is that applications are supposed to know where to look, dconf-editor could also assume that users have a similar knowledge. Or am I assuming too much? These patches are based on your patch-set (not including the search wip). 

Should I attach them to this bug or is there somewhere more appropriate?
Comment 12 Davi 2017-11-20 18:50:48 UTC
relocatable schemas, not patches, ofc.
Comment 13 Arnaud B. 2017-11-21 05:38:38 UTC
(In reply to Davi from comment #11)
> I've bitten the bullet. I've written a tentative of making the model
> dynamic. And also a second patch with a proposal for a way to allow users to
> manually define where the relocatable patches should be used. As the
> justification for the spartan GSettings API is that applications are
> supposed to know where to look, dconf-editor could also assume that users
> have a similar knowledge. Or am I assuming too much? These patches are based
> on your patch-set (not including the search wip). 
> 
> Should I attach them to this bug or is there somewhere more appropriate?

This bug could be used (poor Jakub), but don’t forget that I won’t push anything until there’s a correct (at least as good as currently) search function, that the current search function needs the treeview, and that making the model dynamic needs first to remove the treeview. So, prioritize the new search function. ;)
Comment 14 Davi 2017-11-22 03:27:30 UTC
Created attachment 364160 [details]
Davi's new wip patchset

This is a big, intrusive, barely tested patch-set.

- Make mode dynamic: makes Directories be re-created on every path-request. It is not fully dynamic because it still only parses schemas on startup: new dconf entries should appear dynamically, but not schema changes.

- Add support for manually allocating schemas (this is unrelated to search and should be elsewhere, probably): creates a setting key /ca/dest/dconf-editor/manual-schemas, with type a{ss}, that is inspected when parsing schemas. For each entry, left-side indicates a path spec that allows * for segments and right side is a relocatable schema id. Value example:
{'/org/gnome/desktop/app-folders/folders/*/': 'org.gnome.desktop.app-folders.folder'}

- Add a search results view: it is a preliminary tentative to deliver what was defined in comment #3.
Comment 15 Davi 2017-11-22 03:30:14 UTC
Please, note that the search in the patch above only matches the entire search string against key/folder names (no summary/description matching yet).
Comment 16 Arnaud B. 2017-11-22 21:01:24 UTC
(In reply to Davi from comment #14)
> This is a big, intrusive, barely tested patch-set.

Yay. :)

> - Make mode dynamic: makes Directories be re-created on every path-request.
> It is not fully dynamic because it still only parses schemas on startup: new
> dconf entries should appear dynamically, but not schema changes.

The code is quite different of what I had in mind, and some things at least are not in my mind going in the correct direction:
 * First, you avoid the difficulty, by forgetting all directories that do not have a key edited in it, as you’re basing the creation of subfolder on the dconf client results. That should be directed from SettingsModel.
 * Secondly, you’re moving back to generate directories in directories, and you’ll have a bad time making something dynamic in that direction (I’ve tried, that’s between other things removed in the “Clean model” patch of my patchset).
 * For helping the “there’s a new schema” bug, just check if settings_schema_source.list_schemas() result changed each time a path is requested, and discard cached results if something changed. 

Directory should be a quite minimal class regarding other directories: probably even no link to its Directory children, just knowing at creation the paths it has to display as children; if children change, all is trashed. It should move to be more and more a Schema one, that contains informations mostly on keys.

If you don’t see what I have in mind, please don’t loose time to change that and concentrate on the search UI, that’s highly more important for now.

> - Add support for manually allocating schemas (this is unrelated to search
> and should be elsewhere, probably): creates a setting key
> /ca/dest/dconf-editor/manual-schemas, with type a{ss}, that is inspected
> when parsing schemas. For each entry, left-side indicates a path spec that
> allows * for segments and right side is a relocatable schema id. Value
> example:
> {'/org/gnome/desktop/app-folders/folders/*/':
> 'org.gnome.desktop.app-folders.folder'}

Ah, yes, that’s a fun test. For having your opinion on things that always conflicted in my mind:
 * you’re doing a little danse with schema_already_installed_there, wouldn’t it be easier to have a mapping of schemas to a list of paths/a path spec/a list of path specs, instead of a mapping of a path spec to a schema? It would be easier for users, easier for creating the UI that will manage that, and at least schema names are known to be unique…
 * you cover the application of a schema to a path, to more than one unrelated paths, to all subdirectories of a path, but do you cover also the “/org/project/application/plugins/*/schema1/” case? it’s hard to test, and you know your code better than I do. ^^

> - Add a search results view: it is a preliminary tentative to deliver what
> was defined in comment #3.

Yeps, that’s a great one. Various comments:
 * don’t bind a “global” search_results_model ListStore, else the view is cleaned when the ListStore is, and that does a flickering; instead, create a new GLib.ListStore for each search change, and bind it when it’s populated;
 * add a header for the “local search”; in my non-rebased wip patch, there’s code to have this header appear with searchbar (it’s a revealer);
 * for the “Folders” results, need to see the complete path of folders;
 * for the “Keys” results, I see two possible paths to go:
     1) do as in your initial patch, instead of having one “Keys” header, add one header by folder; or
     2) display the key path in the key’s row;
   having a specific KeyListBoxRow-like widget is needed anyway, to avoid problems in reusing KeyListBoxRowEditable.

(In reply to Davi from comment #15)
> Please, note that the search in the patch above only matches the entire
> search string against key/folder names (no summary/description matching yet).

As long as there’s an usable and useful search, I can push, and other things could be added later. But I think it won’t be so hard to add.
Comment 17 Arnaud B. 2017-11-23 15:02:10 UTC
Created attachment 364277 [details]
Cleaning patchset.

Rebase, one patch split.
Comment 18 Davi 2017-11-23 16:36:19 UTC
What do you think of moving the dynamic model discussion to https://bugzilla.gnome.org/show_bug.cgi?id=731750 and the relocatable schema support discussion to https://bugzilla.gnome.org/show_bug.cgi?id=762803, is that ok?

> Yeps, that’s a great one. Various comments:
>  * don’t bind a “global” search_results_model ListStore, else the view is
> cleaned when the ListStore is, and that does a flickering; instead, create a
> new GLib.ListStore for each search change, and bind it when it’s populated;

Ok.

>  * add a header for the “local search”; in my non-rebased wip patch, there’s
> code to have this header appear with searchbar (it’s a revealer);

Ok.

>  * for the “Folders” results, need to see the complete path of folders;
>  * for the “Keys” results, I see two possible paths to go:
>      1) do as in your initial patch, instead of having one “Keys” header,
> add one header by folder; or
>      2) display the key path in the key’s row;
>    having a specific KeyListBoxRow-like widget is needed anyway, to avoid
> problems in reusing KeyListBoxRowEditable.

Is it really a problem reusing that? I think it might be a nice use case: instead of navigating to the path of the key, just search for it and make the change from the search result itself. I was thinking of giving the existing widgets a "search-result-mode" boolean property that would simply show the path somewhere in the row. Is it too much laziness? :)
 
> As long as there’s an usable and useful search, I can push, and other things
> could be added later. But I think it won’t be so hard to add.

True and true.
Comment 19 Arnaud B. 2017-11-23 16:49:19 UTC
(In reply to Davi from comment #18)
> What do you think of moving the dynamic model discussion to
> https://bugzilla.gnome.org/show_bug.cgi?id=731750 and the relocatable schema
> support discussion to https://bugzilla.gnome.org/show_bug.cgi?id=762803, is
> that ok?

Yeps.

> >  * for the “Keys” results, I see two possible paths to go:
> >      1) do as in your initial patch, instead of having one “Keys” header,
> > add one header by folder; or
> >      2) display the key path in the key’s row;
> >    having a specific KeyListBoxRow-like widget is needed anyway, to avoid
> > problems in reusing KeyListBoxRowEditable.
> 
> Is it really a problem reusing that? I think it might be a nice use case:
> instead of navigating to the path of the key, just search for it and make
> the change from the search result itself. I was thinking of giving the
> existing widgets a "search-result-mode" boolean property that would simply
> show the path somewhere in the row. Is it too much laziness? :)

I think laziness is duplicating KeyListBoxRow or at least its derivative. Because you’ll have a hard time trying to “patch” with your boolean all the functions of KeyListBoxRowEditable, notably its popover (to add a “Show parent folder” in it, probably remove “Copy” in it, etc.). You can try, but I think it would be easier to merge the two versions later than to split them because it’s too hard to maintain.
Comment 20 Davi 2017-11-24 03:39:26 UTC
Created attachment 364305 [details] [review]
Davi's wip search patch

- Add header for "Current folder" results
- Show full full_name for entries not in current folder
- Only show the results (i.e. bind the list model) when the search is over (that means that it may take a few seconds for search terms like "a").
- Add "Open parent folder" action to row popover

For now, I went with the search-result-mode property for folder and key rows, but I created a new RegistrySearch view for the main stack, instead of reusing the RegistryView.

Known issues: 

- Still no proper matching or ranking, only checks if term is in entry name.
Comment 21 Arnaud B. 2017-11-24 10:27:07 UTC
(In reply to Davi from comment #20)
> Created attachment 364305 [details] [review] [review]
> - Add header for "Current folder" results

Make the switch to the RegistrySearch at the moment the search bar is requested (with the header appearing as well, that’s why I’m talking about a Revealer somewhere).

> - Show full full_name for entries not in current folder

Great. :) The “bold” for keys with modified value should really be replaced with an icon, there’s a little too much when the complete path is displayed.

> - Only show the results (i.e. bind the list model) when the search is over
> (that means that it may take a few seconds for search terms like "a").

For when the search entry is empty, only search in the “Current folder” provider (and the “Bookmarks” if you add one). You can also skip for now the “Keys” provider until there are three/four chars. It might be possible in a second time to estimate the number of folder/keys rows that could be displayed, and bind the model when all the view height is taken (when the “Folders” provider has finished its work, notably).

> - Add "Open parent folder" action to row popover

I’m not sure it’s useful for “Folders” results, but it’s really great to have it for the “Keys” results.

> For now, I went with the search-result-mode property for folder and key
> rows, but I created a new RegistrySearch view for the main stack, instead of
> reusing the RegistryView.

Do as you’re comfortable with. Change might be done after, as long as the UI is written and pushed.
Comment 22 Davi 2017-11-26 03:10:46 UTC
(In reply to Arnaud B. from comment #21)
> Make the switch to the RegistrySearch at the moment the search bar is
> requested (with the header appearing as well, that’s why I’m talking about a
> Revealer somewhere).

I made it work with the header function. Please, see if it is ok.

> Great. :) The “bold” for keys with modified value should really be replaced
> with an icon, there’s a little too much when the complete path is displayed.

Could we leave that to be tackled later in https://bugzilla.gnome.org/show_bug.cgi?id=790477 ?

> For when the search entry is empty, only search in the “Current folder”
> provider (and the “Bookmarks” if you add one). You can also skip for now the
> “Keys” provider until there are three/four chars. It might be possible in a
> second time to estimate the number of folder/keys rows that could be
> displayed, and bind the model when all the view height is taken (when the
> “Folders” provider has finished its work, notably).

I improved the way the model is associated and the views are switched, and I think I fixed most of the stuttering. Most notably, "refining searches", where you keep adding characters, are working very smoothly. For searches that remove characters, or otherwise change the search term, there is a noticeable blink, because the model must be repopulated from scratch. In my tests, waiting for the search to finish lead to worse stuttering, making the app feel less responsive. 

I've added a bookmarks section to the results too.
Comment 23 Davi 2017-11-26 03:17:42 UTC
Created attachment 364405 [details] [review]
Davi's wip search patch

- Smother changes in the results list, especially when refining the search
- Bookmarks section
- Show results whenever in search mode, but if the term is empty, only show local and bookmark results

The other patch adds a couple of convenience functions to the SettingsModel class, that could be useful in general.
Comment 24 Arnaud B. 2017-11-26 06:52:38 UTC
Yay, amazing!

(In reply to Davi from comment #22)
> I made it work with the header function. Please, see if it is ok.

There are some little things I don’t understand, but it’s working, so ok. I’d like to work a little more the transitions, but I could do it later.

> > Great. :) The “bold” for keys with modified value should really be replaced
> > with an icon, there’s a little too much when the complete path is displayed.
> 
> Could we leave that to be tackled later in
> https://bugzilla.gnome.org/show_bug.cgi?id=790477 ?

Clearly.

> I improved the way the model is associated and the views are switched, and I
> think I fixed most of the stuttering. Most notably, "refining searches",
> where you keep adding characters, are working very smoothly.

It’s definitively fast enough. Great. :)

> For searches that remove characters, or otherwise change the search term,
> there is a noticeable blink, because the model must be repopulated from
> scratch. In my tests, waiting for the search to finish lead to worse
> stuttering, making the app feel less responsive. 

Ok. There’s no problem visually, just the little lag; it’s good enough for me.

> I've added a bookmarks section to the results too.

Looks like get_object() is failing to do its work (?), it skips all my bookmarked keys. ^^

(In reply to Davi from comment #23)
> The other patch adds a couple of convenience functions to the SettingsModel
> class, that could be useful in general.

Please, no get_parent(). Either do a
  public static string get_parent_path (string path)
or (might be better) keep the parent path as a string in SettingObject. But I’d like to remove as possible relations between SettingObject items.

There are some little things I’d like to change here or there, but more or less all is working “good enough” for me. So please clean as possible, I’ll probably push one of the next versions. :)
Comment 25 Davi 2017-11-26 07:38:49 UTC
(In reply to Arnaud B. from comment #24)
> There are some little things I don’t understand, but it’s working, so ok.
> I’d like to work a little more the transitions, but I could do it later.

Ok.
 
> Looks like get_object() is failing to do its work (?), it skips all my
> bookmarked keys. ^^

Should be fixed now.
 
> Please, no get_parent(). Either do a
>   public static string get_parent_path (string path)
> or (might be better) keep the parent path as a string in SettingObject. But
> I’d like to remove as possible relations between SettingObject items.

I've moved to a get_parent_path, since that is all I needed. But I don't see what is the problem in having that method in the model API, depending on how it is done. If you are OK with get_object and get_parent_path, well, get_parent may be a simple composition of the two, no extra hard connections needed.
Comment 26 Davi 2017-11-26 07:39:45 UTC
Created attachment 364413 [details] [review]
Davi's wip search patch

- Fix bookmark search not showing keys
Comment 27 Davi 2017-11-27 00:26:51 UTC
Created attachment 364470 [details] [review]
Fix miscasting to RegistryView

I've discovered that your patch-set issues a warning when a DConfKey is erased.

(dconf-editor:29373): GLib-GObject-WARNING **: invalid cast from 'BrowserView' to 'RegistryView'

The attached patch seems to fix it.
Comment 28 Arnaud B. 2017-11-28 06:33:30 UTC
I pushed all the patches, thanks for your hard work on this. I’ll start working more on refining the new functions than on adding new things, so expect little changes here and there. But I’ll continue to review patches even on big improvements, of course. :)