[moving dconf>editor tickets to dconf-editor product. See bug 744791]

I've been trying to make a patch for this and discovered that a GSettingsSchemaSource object caches everything, and g_settings_schema_source_get_default also returns the cached instance. Therefore, for added/removed schemas to be perceived, we must create new GSettingsSchemaSource objects using g_settings_schema_source_new_from_directory, mimicking what g_settings_schema_source_get_default does.

Created attachment 364480 [details] [review]
Davi's wip patch-set

Second attempt at making the model dynamic. I've tested and it catches add/remove schemas dynamically. From what I tested up to now, the single lost feature is keeping erased schema-less keys visible. They vanish when the view is refreshed.

This patch-set applies on top of the clean-up patch-set from Arnaud [1].

- First patch makes the model dynamic by re-parsing schemas and dconf on every path request (but that is not enough, because of schema caching)
- Second patch makes the model truly dynamic by recreating the GSettingsSchemaSource on every path request
- Third patch removes content creation from Directory and moves it to the SettingsModel. Child directory has an empty key_model and is only used as a wrapper around its path. I think it would be even better if it would not have a key_model at all (i.e. drop SettingsModel.get_object and use SettingsModel.lookup directly). What do you think?

[1] https://bugzilla.gnome.org/show_bug.cgi?id=768275#c17

Just remembered that the "multiple schemas warning" is also not working with the patch-set above.

(In reply to Davi from comment #2)
> I've been trying to make a patch for this and discovered that a
> GSettingsSchemaSource object caches everything, and
> g_settings_schema_source_get_default also returns the cached instance.

Ha ha. I didn’t dig that far into how it works, and hadn’t this behaviour in mind. But that’s quite logical, usual applications don’t want to deal with g_settings_schema_source_get_default() answering different things in various part of the program. But dconf-editor does. Ouch.

> Therefore, for added/removed schemas to be perceived, we must create new
> GSettingsSchemaSource objects using
> g_settings_schema_source_new_from_directory, mimicking what
> g_settings_schema_source_get_default does.

Having a look to patch 2, that’s maybe not going in the right direction. A change of SchemaSource is not something that common, and asking for many I/O and a quite complex behaviour each time we change path is not a good idea, it will be too slow in some situations.

Better would be to use a cached SchemaSource for usual functions, and have a query each 5 seconds or so to see if the SchemaSource should be updated. With a notification to the user if that has an implication on what is displayed.

(In reply to Davi from comment #3)
> From what I tested up to now, the single lost feature is keeping erased
> schema-less keys visible. They vanish when the view is refreshed.

Erased schema-less keys are mainly for now a placeholder for not being able to update dynamically the whole model. There are two use cases:
 * the user removes a key in the UI;
 * a key is “deleted” externally.
In the first case, it’s quite expected for an user to not see anymore something he just deleted; the current behaviour might be more confusing than useful (notably because you cannot edit the key back, so the item has no real use). In the second case, the usual behaviour of having an user notification “something changed in your view, refresh?” should be enough, even if it’s a little intrusive. So I’d say to go with removing this “feature” if that’s unneeded now.

> - First patch makes the model dynamic by re-parsing schemas and dconf on
> every path request (but that is not enough, because of schema caching)

You’re adding things to Directory, but you’re removing them in patch 3, so ok with that if that’s helping. ^^

There’s a code ~duplication (yes, a cleaning is needed here anyway) in the end of the DConfWindow creation after my patchset, but because of a small behaviour I’d like to keep. With both master and after my patchset, you can run in a terminal `dconf-editor /org/gnome` and finish on “/org/gnome/” [as there’s no “/org/gnome” key]. It’s great for stupid people like me. :) If you can keep it, that would be great (but at least, there’s a correct fallback now).

> - Third patch removes content creation from Directory and moves it to the
> SettingsModel. Child directory has an empty key_model and is only used as a
> wrapper around its path. I think it would be even better if it would not
> have a key_model at all (i.e. drop SettingsModel.get_object and use
> SettingsModel.lookup directly). What do you think?

I need to study more both patch 1 and this patch, but looks like it’s the way to go.

The lookup() and get_base_path() methods could be made private, and looks like “client.watch_sync ("/");” could be done at construct{}. It’s quite possible that SettingsModel finishes with only one public method, and it would be great for my little brain.

(In reply to Arnaud B. from comment #5)
> Having a look to patch 2, that’s maybe not going in the right direction. A
> change of SchemaSource is not something that common, and asking for many I/O
> and a quite complex behaviour each time we change path is not a good idea,
> it will be too slow in some situations.
>
> Better would be to use a cached SchemaSource for usual functions, and have a
> query each 5 seconds or so to see if the SchemaSource should be updated.
> With a notification to the user if that has an implication on what is
> displayed.

I think it is not possible to know if there is a need to reload, unless we reload and check. For adding/removing schemas, we would need to go as far as g_settings_schema_source_list_schemas, but maybe it would be nice to update the view for changes inside the schemas too? For example, an app developer makes a change/fix to a schema, re-installs the app for testing, and dconf-editor automatically picks the fixed schema.

We could go low-level and rely on g_file_monitor_directory and listen to changes in the schema dirs. This would probably be the best performing solution, and would enable precise notifications, but we would be punching through the abstraction ;).

> Erased schema-less keys are mainly for now a placeholder for not being able
> to update dynamically the whole model. There are two use cases:
>  * the user removes a key in the UI;
>  * a key is “deleted” externally.
> In the first case, it’s quite expected for an user to not see anymore
> something he just deleted; the current behaviour might be more confusing
> than useful (notably because you cannot edit the key back, so the item has
> no real use).

Ok.

> In the second case, the usual behaviour of having an user
> notification “something changed in your view, refresh?” should be enough,
> even if it’s a little intrusive. So I’d say to go with removing this
> “feature” if that’s unneeded now.

Ok. I think that could be made to similarly handle any external changes to the current view (e.g. in browser view: sort options changed, key added/removed, schema added/removed/modified; in properties view: key/schema removed or value changed).

> You’re adding things to Directory, but you’re removing them in patch 3, so
> ok with that if that’s helping. ^^

> There’s a code ~duplication (yes, a cleaning is needed here anyway) in the
> end of the DConfWindow creation after my patchset, but because of a small
> behaviour I’d like to keep. With both master and after my patchset, you can
> run in a terminal `dconf-editor /org/gnome` and finish on “/org/gnome/” [as
> there’s no “/org/gnome” key]. It’s great for stupid people like me. :) If
> you can keep it, that would be great (but at least, there’s a correct
> fallback now).

Ok, I'll handle that case. 

In my opinion, the handling of missing paths still has room for improvement. For keys it tries to fallback, but for folders, it always "works", even if the path has nothing inside. You mentioned somewhere that path requests should behave more like searches, so the folder handling might be the more "correct" one. Should missing keys also be handled similarly (i.e. a "placeholder" view with a message  like "There is no key here")? Or maybe use a fallback for folders too?

> > - Third patch removes content creation from Directory and moves it to the
> > SettingsModel. Child directory has an empty key_model and is only used as a
> > wrapper around its path. I think it would be even better if it would not
> > have a key_model at all (i.e. drop SettingsModel.get_object and use
> > SettingsModel.lookup directly). What do you think?
> 
> I need to study more both patch 1 and this patch, but looks like it’s the
> way to go.
> 
> The lookup() and get_base_path() methods could be made private, and looks
> like “client.watch_sync ("/");” could be done at construct{}. It’s quite
> possible that SettingsModel finishes with only one public method, and it
> would be great for my little brain.

Oh, I found another set of features that are not working anymore with the patch-set (e.g. recursive reset, showing badges on changed keys in delayed mode). I'll try to make most of them work for the next iteration.

(In reply to Davi from comment #6)
> I think it is not possible to know if there is a need to reload, unless we
> reload and check.

Cache the “current” SchemaSource for usual work, and “reload and check” in a “watcher”. So that the work will be done in background, without slowing the UI. Doing many I/O and complex things is not a problem here, as long as it doesn’t slow the UI. ^^ Note that there could even be a boolean to disable this repetitive check.

> For adding/removing schemas, we would need to go as far as
> g_settings_schema_source_list_schemas, but maybe it would be nice to update
> the view for changes inside the schemas too? For example, an app developer
> makes a change/fix to a schema, re-installs the app for testing, and
> dconf-editor automatically picks the fixed schema.

Never change view “automatically”, but open an infobar for asking. But yes, it would be great also to handle that case.

> We could go low-level and rely on g_file_monitor_directory and listen to
> changes in the schema dirs. This would probably be the best performing
> solution, and would enable precise notifications, but we would be punching
> through the abstraction ;).

First, let’s have a timer that checks if a refresh is needed, with an UI to notify the user when it happens. In a second time, it will be possible to do something like relying on g_file_monitor_directory, but directly inside glib, that’s not dconf-editor job.

> In my opinion, the handling of missing paths still has room for improvement.

I agree. There’s even one case where it’s “bugging” (using a bookmark to an unexisting folder from a properties-view, IIRC).

> For keys it tries to fallback, but for folders, it always "works", even if
> the path has nothing inside. You mentioned somewhere that path requests
> should behave more like searches, so the folder handling might be the more
> "correct" one. Should missing keys also be handled similarly (i.e. a
> "placeholder" view with a message  like "There is no key here")? Or maybe
> use a fallback for folders too?

When an user requests something, it’s because it has something in mind; so we should try to please it as possible, not saying “hey, you’re stupid”. So, no placeholder –as long as we can have a more or less helpful fallback.

I've been hitting a few obstacles with this approach. The main problem is that the path tree is not kept in memory, so features that rely on that don't work correctly (i.e. will need to be reworked):

- recursive reset (can be fixed by querying the model in the descent, like the search does)
- delayed mode stores planned_change/value in the key object, so recreating key objects leads to inconsistencies. Can be solved by moving the delayed changes state away from the keys to somewhere else (some UI object, ideally)
- search is much slower (lots of memory allocations for sweeping the tree). Could be improved by making key objects more "lazy".

On the other hand, handling schema changes, even with the manual-schemas wildcards, is easy.

An alternative approach is to keep the tree in memory, like the current approach, but "patching" it when schema changes are discovered. This would not have the above issues, but patching the tree in-place is not easy taking manual-schemas into consideration.

I'm not sure which way to go. I think the "tree-less" approach is more elegant, but reworking those features will be tough. Any ideas?

I think you’re trying to do (too) many things at once, and I’m not really surprised you’re horrified by the size of the todo list by then. Even if it’s more work, you’ll probably lose less time by just going step by step, solving one problem after the other.

There will be a need to react to a SchemaSource change anyway (to close this bug at least). And editing “manual-schemas” does more or less the same thing, as it’s modifying the tree. So, I think that one thing that you can do before going all-dynamic is to keep the complete tree for now, but to recreate it each time a SchemaSource/“manual-schemas” change is detected. That’s not something that is expected to happen all the time, so no performance problem.

Recursive resets will then work as for now, and the same for the search. You’re right on that delayed changes state should be moved away from the key, I didn’t had this problem in mind but it will be necessary. Hey, that will look better anyway. This could probably be the first step. Happy hacking, you’re on a really big thing here. ^^

I'm working on removing the key.planned_* stuff, since it seems to be a general improvement that would make the dynamic model easier to deal with whatever the approach. But I faced something I don't understand: <flags>. There are some parts that deal with <flags> keys in a special way, but I cannot figure out the reasons. Particularly, there is this (under RegistryInfo.populate_properties_list_box, also something similar in the revealer.reload.connect):

ulong switch_active_handler = custom_value_switch.notify ["active"].connect (() => {
        if (modifications_handler.should_delay_apply (tmp_string))
        {
            if (custom_value_switch.get_active ())
                modifications_handler.add_delayed_setting (key, null);
            else
            {
                Variant tmp_variant = modifications_handler.get_custom_key_value (key);
                modifications_handler.add_delayed_setting (key, tmp_variant);
                key_editor_child.reload (tmp_variant);
            }
        }
        else
        {
            if (custom_value_switch.get_active ())
            {
                ((GSettingsKey) key).set_to_default ();
                SignalHandler.block (key_editor_child, value_has_changed_handler);
                key_editor_child.reload (key.value);
                if (tmp_string == "<flags>")                /*** why is this needed? ***/
                    key.planned_value = key.value;
                SignalHandler.unblock (key_editor_child, value_has_changed_handler);
            }
            else
                key.value = key.value;  // TODO that hurts...
        }
    });

Why is "key.planned_value = key.value;" done right after [re-]setting the key value? Especially when <flags> do not trigger delayed mode, as per ModificationsRevealer.should_delay_apply (I think it should, but that's another story)?

No idea. Because it wasn’t working correctly without, probably; expectedly or not. Just do your best, and we will test all after; the “Demo” folder is here for that. Yeah, one day, there will be some regression tests running. One day.

Created attachment 364946 [details]
Remove planned_* from Key

With this, Keys are almost totally stateless. Only is_ghost remains, but that seems easier to deal with.

- 0001-Move-sorting-logic-to-BrowserView.patch: Small unrelated refactor, model doesn't need to know about sorting at all

- 0002-Move-set_delayed_icon-inside-KeyListBoxRow.patch: Small refactor. The method is totally related to KeyListBoxRow, so fits better there. Will be further modified later.

- 0003-Move-delayed-changes-logic-into-new-class.patch: Start the big move. Separate "delayed mode" logic into a new class ModificationsHandler. ModificationsRevealer becomes a view for ModificationsHandler. I could have gone with the ModificationsRevealer alone, but I knew it would need to be accessed in even more places then before. Sharing a piece of the UI around can make things messy.

- 0004-Move-set_key_value-into-ModificationsHandler.patch: Small-ish refactor (does a bit more then the title, moves get_current_delay_mode). Those methods are totally delay-related, so fitted better in ModificationsHandler.

- 0005-Move-mods.-handler-instantiation-to-DConfWindow.patch: Small change to make things less convoluted. DConfWindow needs the ModificationsHandler too, so move it and start sharing from there. It is close to the SettingsModel, hinting to a possible merge in the future?

- 0006-Remove-uses-of-planned_-from-key-editors.patch: A small step towards isolating access to those properties. Could probably merge with next patch, since I had to review some if it.

- 0007-Move-all-key.planned_-accesses-to-mods.-handler.patch: Big step, moving every access to planned_* into ModificationsHandler, but sending the handler around a bit to "compensate".

- 0008-Remove-planned_-from-Key.patch: Final move. Now planned changes are stored in the ModificationsHandler's hashtables, so recreating key objects in the future will not lose the info or create inconsistencies.

Happy reviewing! :)

Created attachment 364947 [details]
Remove planned_* from Key

- Rebased

(In reply to Davi from comment #12)
> With this, Keys are almost totally stateless. Only is_ghost remains, but
> that seems easier to deal with.

Great!

> - 0001-Move-sorting-logic-to-BrowserView.patch: Small unrelated refactor,
> model doesn't need to know about sorting at all
> 
> - 0002-Move-set_delayed_icon-inside-KeyListBoxRow.patch: Small refactor. The
> method is totally related to KeyListBoxRow, so fits better there. Will be
> further modified later.
> 
> - 0003-Move-delayed-changes-logic-into-new-class.patch: Start the big move.
> Separate "delayed mode" logic into a new class ModificationsHandler.
> ModificationsRevealer becomes a view for ModificationsHandler. I could have
> gone with the ModificationsRevealer alone, but I knew it would need to be
> accessed in even more places then before. Sharing a piece of the UI around
> can make things messy.
> 
> - 0004-Move-set_key_value-into-ModificationsHandler.patch: Small-ish
> refactor (does a bit more then the title, moves get_current_delay_mode).
> Those methods are totally delay-related, so fitted better in
> ModificationsHandler.
> 
> - 0005-Move-mods.-handler-instantiation-to-DConfWindow.patch: Small change
> to make things less convoluted. DConfWindow needs the ModificationsHandler
> too, so move it and start sharing from there. It is close to the
> SettingsModel, hinting to a possible merge in the future?

First five patch are already pushed. A merge might happen, or they could maybe finish hidden behind an unique interface? Future will say anyway.

> - 0006-Remove-uses-of-planned_-from-key-editors.patch: A small step towards
> isolating access to those properties. Could probably merge with next patch,
> since I had to review some if it.
> 
> - 0007-Move-all-key.planned_-accesses-to-mods.-handler.patch: Big step,
> moving every access to planned_* into ModificationsHandler, but sending the
> handler around a bit to "compensate".

Please merge these two patch, I only understand them when both are applied.

> - 0008-Remove-planned_-from-Key.patch: Final move. Now planned changes are
> stored in the ModificationsHandler's hashtables, so recreating key objects
> in the future will not lose the info or create inconsistencies.

Looks good, I’m doing some testing to find weird regressions. :P

(Patch 8(→7) needs a rebase now, also.)

Created attachment 365081 [details]
Remove planned_* from Key

- Merge previous patches 6 and 7 into a single patch
- Rebase

I pushed (but continue testing!), many thanks for your work! What’s the next step? ^^

Created attachment 365224 [details]
Davi's wip patch-set

The first patch removes direct directory walking, avoiding issues with recursive-reset and search in the next patch. 

The second patch is a rework of the earlier dynamic model patch. It grew kind of "organically" with successive rebases. I could not easily split it into semantically sound steps, so it is a bit large. If you think it is going in the right direction, I can focus on cleaning it up and making it into a more detailed patch-set.

The approach for the dynamic model is what you proposed in comment #5. Schemas are re-checked every few seconds comparing it to the cache. If modifications are discovered, those are signaled and the view shows the reload infobar when appropriate. Only additions and removals are detected for now, but the machinery is in place for handling schema-internal changes (lacking just an algorithm to fingerprint a schema).

In a first moment, I tried to apply that logic to the fully populated directory tree, but that proved too difficult. Therefore, I replaced that with a simpler prefix tree of sorts, where only paths and schemas are cached (i.e. no keys). This new prefix tree helps handle wildcards in paths much more easily. That is a very intrusive change, with Directory keeping only its name/full_name and the "multiple schemas" warning flag. So the directory "views" (i.e. the ListStore's) are created per-request with the aid of the cached prefix tree.

DConfKey's are no longer "ghosted". Instead, when such key changes or is removed, a view reload is triggered. That is only half-done though, and has issues: Every non-schema key modification asks for a view reload (should be only external modifications). On the other hand, modifications of keys with schemas never ask (external modifications should). There are likely more problems.

Impossible to find “/org/gnome/Epiphany/”. ^^ (With a “E”, Epiphany use two folders, the one with “e” also. That’s its own problem.) And “/apps/”, “/desktop/” and “/system/”.

The first patch looks good, can I push it or do you want to make changes on it before?

Still reading the second patch. But I like the look of the Directory class. ^^

I'm working to improve these patches, making them less chaotic. You may want to defer the reading a bit.

Created attachment 365280 [details]
Davi's wip search patch

Patch-set a bit more step-by-step and with hopefully more readable diffs.

Created attachment 365281 [details]
Davi's wip patch-set

Fix silly bug...

Created attachment 365283 [details]
Davi's wip search patch

- Fix issue /org/gnome/Epiphany/ (or any path with no schema or DConf content, but with schema-discovered subpaths)

Thinking. If create_schema_source() returns a SchemaSource from a different “path” (one of the system_data_dirs / the user_data_dir / the GSETTINGS_SCHEMA_DIR variable), we have big problems. All of the current delayed changeset may not mean anything after the SchemaSource change; same for the current path. Etc.

That’s an important information to check I think, but if something changed there, the best we can do is probably to say the user “something drastically changed in your configuration, please save your changes, close and reopen”. That doesn’t change of course that rest of the work done by the Timeout.

One little thing I’ve in mind:
  application_settings.changed ["relocatable-schemas-user-paths"].connect
should probably check if RelocatableSchemasEnabledMappings.USER is enabled.

(In reply to Arnaud B. from comment #24)
> Thinking. If create_schema_source() returns a SchemaSource from a different
> “path” (one of the system_data_dirs / the user_data_dir / the
> GSETTINGS_SCHEMA_DIR variable), we have big problems. All of the current
> delayed changeset may not mean anything after the SchemaSource change; same
> for the current path. Etc.
> 
> That’s an important information to check I think, but if something changed
> there, the best we can do is probably to say the user “something drastically
> changed in your configuration, please save your changes, close and reopen”.
> That doesn’t change of course that rest of the work done by the Timeout.

I thought it was not possible to change the environment variables of a running process. I might be wrong though.

> One little thing I’ve in mind:
>   application_settings.changed ["relocatable-schemas-user-paths"].connect
> should probably check if RelocatableSchemasEnabledMappings.USER is enabled.

Ok.

(In reply to Davi from comment #25)
> I thought it was not possible to change the environment variables of a
> running process. I might be wrong though.

I think you’re right (I think), you can skip checking path change if GSETTINGS_SCHEMA_DIR is used and just update the source. But checking if something (related to settings source) changed in the system data dirs or the user data dir always looks like an important information.

I’m stupid. You also get an env variable for get_system_data_dirs() etc. Isn’t there a way to know if something changed there?!

So, let’s say there’s no way. Then instead of calling each time create_schema_source(), you can save the first time which “SchemaSource source” is used, and directly regenerate the SchemaSource from here.

(In reply to Arnaud B. from comment #28)
> So, let’s say there’s no way. Then instead of calling each time
> create_schema_source(), you can save the first time which “SchemaSource
> source” is used, and directly regenerate the SchemaSource from here.

From what I could understand, the environment variables don't change, so the paths won't change. Nevertheless, the contents of said paths may change. What if the user creates a $XDG_DATA_HOME/glib-2.0/schemas/ dir and installs/compiles schemas in it? Dconf Editor should pick those up, even if the directory didn't exist before.

Hmm... thinking about schema sources I've got an idea. What about supporting user-defined schema sources? GSETTINGS_SCHEMA_DIR allows only a single directory (enough for normal apps), so Dconf Editor could allow the user to specify a list of directories where to look for schemas. The use-case I have in mind is GNOME Shell extensions, which install schemas in their own folders. Say, tell Dconf Editor to look for every path under ~/.local/share/gnome-shell/extensions/ containing a gschemas.compiled. What do you think?

(In reply to Davi from comment #29)
> From what I could understand, the environment variables don't change, so the
> paths won't change. Nevertheless, the contents of said paths may change.

You’re right.

> What if the user creates a $XDG_DATA_HOME/glib-2.0/schemas/ dir and
> installs/compiles schemas in it? Dconf Editor should pick those up, even if
> the directory didn't exist before.

Back to my initial thinking. I’m not sure we should “pick those up”, because if the SchemaSource origin path changes, things really complex could happen. We should only ask to the user to save its change and restart the app if that happens.

(In reply to Davi from comment #30)
> Hmm... thinking about schema sources I've got an idea. What about supporting
> user-defined schema sources? GSETTINGS_SCHEMA_DIR allows only a single
> directory (enough for normal apps), so Dconf Editor could allow the user to
> specify a list of directories where to look for schemas. The use-case I have
> in mind is GNOME Shell extensions, which install schemas in their own
> folders. Say, tell Dconf Editor to look for every path under
> ~/.local/share/gnome-shell/extensions/ containing a gschemas.compiled. What
> do you think?

One complex beast at the time. The “org.gnome.Epiphany.permissions” has been added to skipped_schemas because it uses a different backend that dconf (a keyfile one), and I’m sure it will be fun to try to support it. But let’s keep this kind of things for later.

(In reply to Arnaud B. from comment #31)
> Back to my initial thinking. I’m not sure we should “pick those up”, because
> if the SchemaSource origin path changes, things really complex could happen.

What kind of things do you foresee? Right now I cannot think of anything that doesn't translate to one of (or a combination of) "a new schema is added", "a schema is removed" or "a schema is modified".

> We should only ask to the user to save its change and restart the app if
> that happens.

I understood the purpose of the dynamic model would be that the "Reload" action should have the same effect as restarting the app.

> One complex beast at the time. The “org.gnome.Epiphany.permissions” has been
> added to skipped_schemas because it uses a different backend that dconf (a
> keyfile one), and I’m sure it will be fun to try to support it. But let’s
> keep this kind of things for later.

Of course. I was just throwing the idea. I have too many dconf-editor branches in my local repo already :).

(In reply to Davi from comment #32)
> What kind of things do you foresee? Right now I cannot think of anything
> that doesn't translate to one of (or a combination of) "a new schema is
> added", "a schema is removed" or "a schema is modified".

I’m fearing a “most of the tree is removed because something got wrong at main schemas path”, but you’re probably right, `dconf-editor` should probably help to see even what’s going then.

> I understood the purpose of the dynamic model would be that the "Reload"
> action should have the same effect as restarting the app.

That’s the goal, but there’s this little corner-case of pending changes, that is hard to manage from a UI point of view. But maybe a “dismiss pending changes and reload” could do the job.

> > One complex beast at the time. The “org.gnome.Epiphany.permissions” has been
> > added to skipped_schemas because it uses a different backend that dconf (a
> > keyfile one), and I’m sure it will be fun to try to support it. But let’s
> > keep this kind of things for later.
> 
> Of course. I was just throwing the idea. I have too many dconf-editor
> branches in my local repo already :).

The whole gsettings/dconf/others work is full of little subtleties that will be hard to cover completely anyway. So at one point, I’ll just take what’s done and maintainable. ^^

Apart the mentioned check of RelocatableSchemasEnabledMappings.USER, please use build_filename for building filenames, and add in a comment on top of create_schema_source() an URL to the glib code[1]. 

The patchset looks already great. Do you think it is ready to be pushed, or do you see some problems in it? If it’s good enough, I’d like to have it in the unstable release of tomorrow.

[1] https://git.gnome.org/browse/glib/tree/gio/gsettingsschema.c#n332

(In reply to Arnaud B. from comment #34)
> Apart the mentioned check of RelocatableSchemasEnabledMappings.USER, 

Done.

> please
> use build_filename for building filenames, 

Done.

> and add in a comment on top of
> create_schema_source() an URL to the glib code[1]. 

Done.

> The patchset looks already great. Do you think it is ready to be pushed, or
> do you see some problems in it? If it’s good enough, I’d like to have it in
> the unstable release of tomorrow.

Well, there are issues that should be worked upon. There's the one I mentioned in comment #18 (i.e. triggering view reloads on external key changes). There is also a related corner case: recursively resetting a directory subtree with DConf-only keys does not trigger a view change, despite removing the subtree. There is likely to be others like that, considering the extension of the changes. If you are ok with patching those later, the chance of having more feedback with the release is welcome. I'll keep hanging around, so you may CC me  on the bug reports, ofc.

Created attachment 365324 [details]
Davi's wip patch-set

- Only reload user relocatable schema mappings when they are enabled
- Use Path.build_filename to ... build filenames
- Add reference to GLib code in create_schema_source
- Fix the name of this attachment :)

(In reply to Davi from comment #35)
> If you are ok with patching those later, the chance of having more feedback
> with the release is welcome.

That’s the spirit, yes. Even if I don’t really expect many people to test enough to find some corner-case bugs, the announcement of tomorrow might motivate some to have a look.

> I'll keep hanging around, so you may CC me keep hanging around, so you may
> CC me on the bug reports, ofc.

In your Preferences > Email Preferences, there’s an “User Watching” section, where you could (if you want, of course!) watch the “dconf-editor-maint@gnome.bugs” technical user; you’ll be then added automatically to new bugs, IIRC at least.

I've made some tests and I think I understand better how DConfClient works. It seems that handling Dconf changes properly requires dealing with tags: take the tag generated by write_sync or change_sync and match it against the tag received in the changed signal. If the tags match, it means the change is triggered by Dconf Editor UI itself, so we may apply the change instantly. Otherwise, show the infobar.

The problem is write_sync and change_sync tags are received by DConfKey and ModificationsHandler, respectively, while changed signal tags are received by the SettingsModel (or Directory, before the dynamic model patch-set). This makes it hard to solve. Ideally they should be all in the same place.

In order to fix it not-hackishly, I thought the model could go through another refactoring that moves all DConfClient accesses into SettingsModel. And while we are at it, also make Key objects more slender in the process.

Created attachment 365332 [details]
Davi's wip patch-set 2: "DConf dynamics"

- Fix showing reload for in-app DConf changes
- Reload immediately (without asking) after in-app DConf changes
- Remove value-related properties and methods from Key objects and move them to SettingsModel

In fact, you might have some time to fix all you want: for now, vala master crashes dconf-editor at startup, and I’ll probably delay the push until I’m confident there’s no problem remaining due to the recent big change in it. Happy hacking until then. ^^

So, just for reminder, I pushed your first patchset in 3.27.3, released early this morning. (There’s been an important Vala change done, but the crashes were more rough edges to polish than a real problem in the new way to do things.) And I’m studying the second patchset. ^^

I'm actually redoing the last two commits of the second patch-set. They have some situations where a needed reload is missed. I've thought of a more robust (i.e. brute-force :) way to decide when to reload the view. I'm short of time in the next days, so it's probably coming by the end of the week.

Created attachment 365515 [details]
Davi's wip patch-set 2: "DConf dynamics"

- Rework the "need reload" checks. Compare the currently viewed content with fresh content taken from the model.
- Make DConfKey behavior more similar to GSettingsKey (i.e. non-erase external value changes trigger a simple value_changed signal and not "need reload")

Yay. :) It’s not easy to understand all the modifications, but the behaviour looks good, and what I’ve read on the code also.

A little thing, while I continue to study all that: when erasing a dconf-key from the properties view, it would be great to have a different message (in-app notification) than “Impossible to find key %s here" (or even no message at all), as it’s a hopefully know information.

Another thing: when using (on a DConfKey) the “Do not erase” action in the right click popover, there’s a “invalid cast from 'DConfKey' to 'GSettingsKey'” warning that’s not in master.

*hardtests all the things*

Created attachment 365571 [details]
Davi's wip patch-set 2: "DConf dynamics"

- Fix mis-casting to GSettingsKey when dismissing a delayed change
- Remove is_ghost property too, replacing its use in SettingsModel.is_key_ghost with a DConfClient query (this fixes a CRITICAL when removing a DConfKey while showing it in the properties view)
- Fix reload-without-ask when removing DConfKey externally while showing the same key in properties view
- Improve DConfWindow.request_path: remove check for ghost keys (freshly queried keys are never ghost); tries to fallback parent-by-parent; additional parameter for suppressing "missing key/folder" notification
- Do not show "missing key" notification if key removed from in-app action

Keys without schemas are never displayed after being removed, right? If I followed all, the “ghost” thing is only useful now because the pathbar cannot be updated and should answer something smart, no? (that fails anyway if the key is the only one in its folder, but that’s a detail.) If so, you probably could try to clean all that also (an “invalidate_after” call for the pathbar, maybe? and no more ghosts; including in CSS file).

The patchset looks great, are you OK to push it in one of the next iterations, or do you want to work more on it?

(In reply to Arnaud B. from comment #47)
> Keys without schemas are never displayed after being removed, right? If I
> followed all, the “ghost” thing is only useful now because the pathbar
> cannot be updated and should answer something smart, no? (that fails anyway
> if the key is the only one in its folder, but that’s a detail.) If so, you
> probably could try to clean all that also (an “invalidate_after” call for
> the pathbar, maybe? and no more ghosts; including in CSS file).

Actually, there are a few situations where a key presented in the view is "ghost", but they will only occur if the key is removed externally (e.g. "dconf reset ..."):

- In the RegistryView, the displayed value becomes "Key erased." (the "empty set" icon is not showing, a regression probably) and there is not much that can be done with it (it should also become "insensitive", I think, because activating it does not "work"). 

- In the RegistryInfo (a.k.a properties view) the key value also becomes "Key erased." but the key may be "resurrected" by applying the value in the editor

- If the key has a delayed change, although it is not shown explicitly, applying the change will also resurrect it.

I think the possibility of resurrecting a key might be interesting and gives a bit more value to the warning, but I'm not sure. We could go either way:

- Once a DConfKey is erased, remove all traces of it (entries in the pathbar, the editor in the properties view and possible delayed changes, I think)

- Allow it to be redefined by a few limited actions (mostly how it is done by the current patch-set, but some improvements could be made). We could even show a "ghost" key in the RegistryView if there is a delayed change for it, whether it exists in the model or not. 

What do you think? Kill the ghosts or let them linger?
 
> The patchset looks great, are you OK to push it in one of the next
> iterations, or do you want to work more on it?

I'm OK. It is getting a bit big and rewriting my local history to fix regressions is becoming mildly painful. We could fix the remaining ones in further patches.

(In reply to Davi from comment #48)
> Actually, there are a few situations where a key presented in the view is
> "ghost", but they will only occur if the key is removed externally (e.g.
> "dconf reset ..."):

Looks like I forgot to re-test that, I was mostly concentrated on adding/removing schemas. Too much changes recently for my little brain. ^^’

> I think the possibility of resurrecting a key might be interesting and gives
> a bit more value to the warning, but I'm not sure.

It’d be hard to explain why we could re-edit a removed key, but not create one from scratch. And to explain why we would not allow to select the type of a key at its resurrection. And DConf keys should not be used now, applications should use GSettings API.

So no way to “edit a ghost key”, and nothing that is presented as a way to “resurrect” it. But I’d like to go with an “undo” function for every operation, that could be kept in a history, along with visited paths/searches. That does the same, but that’s conceptually different. ^^

> What do you think? Kill the ghosts or let them linger?

I’m trying these days to rethink a little the design. What I have in mind for now is:

 * when you remove one DConf key, either from RegistryInfo or from RegistryView,
   * no ModificationsRevealer, but a “key %s has been removed [undo] | [×]” in-app notification;
   * the “undo” function is kept in a history, along with path visited (and searches, when it’ll be doable); but no need to keep a ghost in the model, just keep path and value as parameters of the GAction of the button;
   * for the pathbar, I think I wouln’t like the path to be “rewritten”; I’d like to improve the behaviour of having a notification when you click on it that says that “the key has been removed”/that “all the keys in the directory have been removed”, but it would be great to have also a visual indication in the pathbar of the “obsolete” paths (greyed, probably);
   * if the parent directory still exists, go there, and the key has disappeared; else, go in the first parent directory that still exists;

 * I’d like the “delayed mode” to become more a “selection mode”[1], so with enough confirmations not to have an in-app notification; but an “undo” in the history, for all the changes done; (for information, that would need an editable pathbar first, because else searchbar looks weird.)

 * the “ghost key” row is needed for external change in RegistryView, but it is a corner-case; I’d go with removing the key from the model, a “reload view” infobar, and an error message if the key is clicked anyway; (but that needs to be able to set GActions to rows, to not go in a signals-hell; that’s bug 741633.)

 * and I bug on how to explain properly in RegistryInfo that the key has been remotely removed; but probably keeping the edition panel is good enough for this uncommon case.

[1] https://developer.gnome.org/hig/stable/selection-mode.html.en

> I'm OK. It is getting a bit big and rewriting my local history to fix
> regressions is becoming mildly painful. We could fix the remaining ones in
> further patches.

I’ll push your next proposition then. Thanks for your work!

(In reply to Arnaud B. from comment #49)
> It’d be hard to explain why we could re-edit a removed key, but not create
> one from scratch. And to explain why we would not allow to select the type
> of a key at its resurrection. And DConf keys should not be used now,
> applications should use GSettings API.

Except for the last one, I think all the others are hard to explain because they "could" actually be done :). The last one is easier to state and explains all the others, though. I'd go with that (maybe add a link to a F.A.Q somewhere?).
 
> So no way to “edit a ghost key”, and nothing that is presented as a way to
> “resurrect” it. But I’d like to go with an “undo” function for every
> operation, that could be kept in a history, along with visited
> paths/searches. That does the same, but that’s conceptually different. ^^

I think having navigation history and undo functionality would be great. But I'm not sure about mixing view state (navigation history) with model state (undo) in the same UI. Especially merging "Ctrl+Z" with "Alt+Left" could be disastrous.

> I’m trying these days to rethink a little the design. What I have in mind
> for now is:
> 
>  * when you remove one DConf key, either from RegistryInfo or from
> RegistryView,
>    * no ModificationsRevealer, but a “key %s has been removed [undo] | [×]”
> in-app notification;
>    * the “undo” function is kept in a history, along with path visited (and
> searches, when it’ll be doable); but no need to keep a ghost in the model,
> just keep path and value as parameters of the GAction of the button;

I think this could work very well and goes in the same direction the HIG: replacing confirmations with undos. Especially when there is the "One confirmation to rule them all" ;).

>    * for the pathbar, I think I wouln’t like the path to be “rewritten”; I’d
> like to improve the behaviour of having a notification when you click on it
> that says that “the key has been removed”/that “all the keys in the
> directory have been removed”, but it would be great to have also a visual
> indication in the pathbar of the “obsolete” paths (greyed, probably);

This might require some reworking of the pathbar, but would be very nice.

>    * if the parent directory still exists, go there, and the key has
> disappeared; else, go in the first parent directory that still exists;

That's how the fallback code in the patch-set works.

>  * I’d like the “delayed mode” to become more a “selection mode”[1], so with
> enough confirmations not to have an in-app notification; 

I don't understand this. The way I see selection mode and selection in general, is that you select a set of items to apply the same action to all of them. In delayed mode, one needs to specify a different value of a different type for each item, and they even might be in different locations. 

Or thinking a bit more, do you mean just the "modes" thing, with a colored header bar and a different set of buttons? We could ask the theme designers to help choose a different color. Would avoid confusion with selection mode and make this feature and Dconf Editor more remarkable and exclusive :).

> but an “undo” in
> the history, for all the changes done; 

This is doable, but dealing with external changes could be a challenge. Maybe give it a slightly different connotation: instead of "Undo modifications", call it "Restore previous state", because that action could undo not just the in-app actions, but also possible external modifications.

> (for information, that would need an
> editable pathbar first, because else searchbar looks weird.)

Could you explain this better? Are you talking about the visual of "delayed mode header" bar with a search bar? About the pathbar, do you mean breadcrumbs-style or full-text style? Another idea is to put both the pathbar and the search entry in the same GtkStack (while showing the current path beside "Current folder" in the search-results header to compensate for hiding it in the header bar).

>  * the “ghost key” row is needed for external change in RegistryView, but it
> is a corner-case; I’d go with removing the key from the model, a “reload
> view” infobar, and an error message if the key is clicked anyway; 

I think this is also working like that in this patch-set.

The model does not have Key objects anymore. It only creates them in response to queries. When we know about an external change, the key is already gone from the model. Although there are still ghost keys, they are different than before: they are part of the view state only. But that view state is informed enough to show the key details and even provide an undo of sorts, sometimes (not in the RegistryView, I think, the value is not stored there).

> (but that
> needs to be able to set GActions to rows, to not go in a signals-hell;
> that’s bug 741633.)

Fixing that bug would be great. But until then... signals-hell it is :).

>  * and I bug on how to explain properly in RegistryInfo that the key has
> been remotely removed; but probably keeping the edition panel is good enough
> for this uncommon case.

Ok. I'll try to sketch some warning message.

(In reply to Davi from comment #50)
> maybe add a link to a F.A.Q somewhere?

Most users do not understand the difference between gsettings and dconf; and most developers just use the GSettings functions because it’s cool. So I’d mostly go for a “don’t let anybody think something here could be done, and nobody will ask.” Even if some Mate users sometimes will.

Yes, the name “dconf-editor” is unfortunate to make dconf hides completely and disappear silently, but I cannot find for what to change it for now.

> I think having navigation history and undo functionality would be great. But
> I'm not sure about mixing view state (navigation history) with model state
> (undo) in the same UI. Especially merging "Ctrl+Z" with "Alt+Left" could be
> disastrous.

Of course, “go back” (<Alt>Left) should not undo. But presenting both in an “history” list doesn’t look like a big problem in my mind (and I do want an “history” list, as detailed on bug 790527); but I may be mistaken, of course. But…

…if that doesn’t look cool, we can have two different lists in the “history” popover, one for things to undo, and one for paths. Or…

…just keep the last undo there. In fact, my main problem is that having an in-app notification can be closed “inadvertently” (it’s taking space, so close it, and oops), so I’d like to at least keep a way to undo somewhere where it cannot be closed (and not only via keyboard shortcut).

> This might require some reworking of the pathbar, but would be very nice.

I’d really like to keep the pathbar code “simple” (can I say “mechanical”?), but marking the obsoletes paths is definitively important.

> >  * I’d like the “delayed mode” to become more a “selection mode”[1], so with
> > enough confirmations not to have an in-app notification; 
> 
> I don't understand this. The way I see selection mode and selection in
> general, is that you select a set of items to apply the same action to all
> of them. In delayed mode, one needs to specify a different value of a
> different type for each item, and they even might be in different locations. 
> 
> Or thinking a bit more, do you mean just the "modes" thing, with a colored
> header bar and a different set of buttons? We could ask the theme designers
> to help choose a different color. Would avoid confusion with selection mode
> and make this feature and Dconf Editor more remarkable and exclusive :).

I do think that most people use the delayed mode when trying to “clean” a directory, and that setting multiple different values is more an exception than a common practice. Of course, that’s hard to know precisely. :·( Assuming that, present this “mode” like a selection should do the (most common) job.

But yes, I have in mind to go with a colored headerbar (all discussion about color until now always finished with a “let’s use Adwaita-blue like always”, we’ll see…), and –if I don’t like the “change all the buttons” spirit of the HIG– importantly: no way to close the window without applying/discarding explicitly pending changes.

That’s what explains by the way the “move copy-path function to the pathbar”, there’s just delayed-mode things in the window menu now (except a placeholder copy function in RegistryInfo, that could be removed).

> This is doable, but dealing with external changes could be a challenge.
> Maybe give it a slightly different connotation: instead of "Undo
> modifications", call it "Restore previous state", because that action could
> undo not just the in-app actions, but also possible external modifications.

You’re clearly right on the terminology. :)

> > (for information, that would need an
> > editable pathbar first, because else searchbar looks weird.)
> 
> Could you explain this better? Are you talking about the visual of "delayed
> mode header" bar with a search bar?

Yeps.

> put both the pathbar and the search entry in the same GtkStack (while
> showing the current path beside "Current folder" in the search-results
> header to compensate for hiding it in the header bar).

Yeps. :)

> I think this is also working like that in this patch-set.

I don’t played enough with the code to understand it fully, but that’s highly possible. I was just copying my mental note about how things should work, not the one about what should be changed. ;)

> >  * and I bug on how to explain properly in RegistryInfo that the key has
> > been remotely removed; but probably keeping the edition panel is good enough
> > for this uncommon case.
> 
> Ok. I'll try to sketch some warning message.

Great. Thanks. :)

Created attachment 365625 [details]
Davi's wip patch-set 2: "DConf dynamics and more..."

- Patches 0001-0009 are the same as before
- 0010 does some simple refactoring
- 0011 makes "ghost" paths to be dimmed in the pathbar (I tried using the "greyed-label" style, but is was too little change, so I went with ... "dim-label" :). They also "undim" when "resurrected".
- 0012 adds different messages for different situations in the reload infobar. The messages themselves might need improvement.

Great! I pushed patch 1~10. :)

I discovered one bug after pushing (always): launching:
   dconf write /ca/desrt/dconf-editor/Demo/test/truc \'true\';
   dconf-editor /ca/desrt/dconf-editor/Demo/test; # no '/' at the end
opens in a weird state, as it considers it’s on a key but is in fact on a folder.

For patch 11:
 * The greying on obsolete paths works great, but not the update of cursor; you have first a “let’s click” cursor, then after a first test, a “there’s a menu” one, and it’s not a good idea to click in any case, and there’s no menu either. Not sure if should be used “help”, “not-allowed” or “default”, needs testing.
 * Maybe the ‘/’ between two obsoletes elements should be even lighter/transparent, or the same color others ‘/’, but that’s bugging me a little.
 * When the window is on :backdrop state (when there’s another window on top of it), obsoletes path look “evanescent”, making all path hard to parse. It’s possible that having them the same color as the rest of the text would be the good solution. (Knowing about the status of a path element is not exactly important if you even don’t look at the window, is it?)

For patch 12:
 * Not tested for now, but I have the feeling that offering to “reload the view” when you know that you’ll be then moved to another folder is not a good idea (the view is more or less the content of the current path in my mind).
 * Might need a bigger refactoring, I’m testing some things around that.

I’m keeping this bug open as there’re patches discussed in it, but also created bug 791694 for adding to the GLib the needed functions for a proper support of this functionality.

(In reply to Arnaud B. from comment #53)
> I discovered one bug after pushing (always): launching:
>    dconf write /ca/desrt/dconf-editor/Demo/test/truc \'true\';
>    dconf-editor /ca/desrt/dconf-editor/Demo/test; # no '/' at the end
> opens in a weird state, as it considers it’s on a key but is in fact on a
> folder.

Fixed. I had to move a few things around and integrate some code paths. The third patch description explains a bit.
 
> For patch 11:
>  * The greying on obsolete paths works great, but not the update of cursor;
> you have first a “let’s click” cursor, then after a first test, a “there’s a
> menu” one, and it’s not a good idea to click in any case, and there’s no
> menu either. Not sure if should be used “help”, “not-allowed” or “default”,
> needs testing.

Fixed. 

Personally, I find the "context-menu" cursor weird. I've never seen it before, I think. In my opinion, it is better to avoid introducing unusual symbols, unless they are accompanied by a descriptive text.

On the other hand, if the greyed items should not be clicked, maybe we could use either "sensitive = false" or even remove them?

>  * Maybe the ‘/’ between two obsoletes elements should be even
> lighter/transparent, or the same color others ‘/’, but that’s bugging me a
> little.
>  * When the window is on :backdrop state (when there’s another window on top
> of it), obsoletes path look “evanescent”, making all path hard to parse.
> It’s possible that having them the same color as the rest of the text would
> be the good solution. (Knowing about the status of a path element is not
> exactly important if you even don’t look at the window, is it?)

Fixed.

> For patch 12:
>  * Not tested for now, but I have the feeling that offering to “reload the
> view” when you know that you’ll be then moved to another folder is not a
> good idea (the view is more or less the content of the current path in my
> mind).
>  * Might need a bigger refactoring, I’m testing some things around that.

I've rebased on top of your modifications and changed the messages a bit. Now it better indicates what is about to happen when the button is clicked. That makes the "missing" notifications redundant, so I removed them.

> I’m keeping this bug open as there’re patches discussed in it, but also
> created bug 791694 for adding to the GLib the needed functions for a proper
> support of this functionality.

Ok, but that bug has Product: dconf-editor, but it proposes patching glib. Is that correct?

Created attachment 365664 [details]
Patch-set 3 (overlaps with last patches in patch-set 2)

(In reply to Davi from comment #54)
> Personally, I find the "context-menu" cursor weird. I've never seen it
> before, I think. In my opinion, it is better to avoid introducing unusual
> symbols, unless they are accompanied by a descriptive text.

I do agree it is quite unusual, but it is codified, so if something has to be changed, it’d be in the gtk+ library (even if it’s only a design change of the Adwaita cursor).

One interest I have in using it here is that it allows discovering the popover, that’d be quite invisible without it. The other way would be to add a down arrow somewhere… where?

> On the other hand, if the greyed items should not be clicked, maybe we could
> use either "sensitive = false" or even remove them?

It’s a grey area here. ^^’ I don’t like removing them as it’s making the pathbar act differently as usual: when you go in a parent directory, it doesn’t forget where you’re coming from; why would it be different when a key is removed? Having the pathbar change its text when you remove multiple keys including the last one visited, but not when the last one has a schema, would be quite bad I think.

And making buttons insensitive is making hard to understand/remember why there’re like that.

> Ok, but that bug has Product: dconf-editor, but it proposes patching glib.
> Is that correct?

Not sure how other maintainers do usually when they plan to update a function of their application by first changing the underlying library. Let’s say that’s a bug to check that the proposed functionality in the GLib will really be usable in `dconf-editor`. And there will be a need for another bug in the GLib, that I let who-wants-to-work-on-that (you? :) ) open.

I’ve opened bug 791775 for the last unpushed patch, about the various messages (and globally the bugs around infobars). For other things, looks like the bug could be closed, even if there’s probably some remaining things to correct, as it was a big change. ^^ So let’s open other bugs when they are found.

Many thanks for your work on this. The function will be available in the 3.28 release, planned IIRC for March 2018.