I think the hard issue with showing multiple schemas is conflicting keys. What should Dconf Editor do with keys that have the same path (besides showing a big glowing scary warning sign :)? I think of two possibilities:

- be pessimistic and "lock-down" any conflicting key (even maybe the accompanying schema), forbidding modifications or even hiding the value, practically forcing the user to see it as an application issue
- be optimistic and check if the keys are actually compatible, giving progressively less scary warnings:
* Incompatible types (ranges?): keys on lock-down, "something really bad might happen" warning
* Compatible types, different descriptions (defaults, ranges?): modifications allowed, "something bad might happen" warning
* Identical keys: modifications allowed, "look, how curious!" warning

On actual implementation, I think the new model makes it very easy to support multiple schemas (minus the "group by schema" UI change). But before that, the "request_path" call should be changed to a "request_descriptor" of sorts, so that key requests could include both the schema ID and the path, to allow addressing conflicting keys individually.

(In reply to Davi from comment #1)
> I think the hard issue with showing multiple schemas is conflicting keys.
> What should Dconf Editor do with keys that have the same path

Note that there are two different problems: the one in the RegistryView, and the one in the RegistryInfo. Solving the first doesn’t require to solve the second completely, even if this would be better of course. ^^ And I think you’re mixing the two in your comment, where my main concern for now is the first one.

> On actual implementation, I think the new model makes it very easy to
> support multiple schemas (minus the "group by schema" UI change).

The “group by schema” thing allows to properly close the RegistryView problem; but it’s not necessary, if that’s difficult to do. There can also be an italic summary “At least two schemas provide different summary for this keys” (if the summaries differ), with the disabling of editing possibilities from RegistryView (or only allowing them is keys are completely compatible). Certainly with a warning icon, anyway.

Here, I think you’re talking about the RegistryInfo problem:

> I think of two possibilities:
> - be pessimistic and "lock-down" any conflicting key (even maybe the
> accompanying schema), forbidding modifications or even hiding the value,
> practically forcing the user to see it as an application issue
> - be optimistic and check if the keys are actually compatible, giving
> progressively less scary warnings:
> * Incompatible types (ranges?): keys on lock-down, "something really bad
> might happen" warning
> * Compatible types, different descriptions (defaults, ranges?):
> modifications allowed, "something bad might happen" warning
> * Identical keys: modifications allowed, "look, how curious!" warning

There could be two “set to default” buttons if there are two defaults provided by different schemas (the exact UI will need discussion). So I’d only count two cases: keys are of the same type (including similar ranges), or not. In the second case, locking doesn’t look like a bad solution, that’s definitively a dangerous case, and something is clearly going wrong. In the first case, we should allow the user to read the different summaries/descriptions and choose to edit or not.

(In reply to Arnaud B. from comment #2)
> Note that there are two different problems: the one in the RegistryView, and
> the one in the RegistryInfo. Solving the first doesn’t require to solve the
> second completely, even if this would be better of course. ^^ And I think
> you’re mixing the two in your comment, where my main concern for now is the
> first one.



> The “group by schema” thing allows to properly close the RegistryView
> problem; but it’s not necessary, if that’s difficult to do. 

It's not hard to group by schemas. I only mentioned that because as I was refactoring for the dynamic model I had to actively add code to stop showing keys from multiple schemas in the RegistryView. But even without that, it could only show one of multiple conflicting keys in the RegistryInfo, because request_path only takes a path. Clicking either key item would always show the same one.

> There can also
> be an italic summary “At least two schemas provide different summary for
> this keys” (if the summaries differ), with the disabling of editing
> possibilities from RegistryView (or only allowing them is keys are
> completely compatible). Certainly with a warning icon, anyway.

It seems to me that it is better to not merge keys from different schemas a into single item/properties view, irrespective of how similar they are. I think it will be much harder to understand the warnings and what is happening if we show a single item. I'd rather treat them as individual keys everywhere.

> Here, I think you’re talking about the RegistryInfo problem:

True, but lock-down should apply to RegistryView too (boolean switches and context popovers).
 
> There could be two “set to default” buttons if there are two defaults
> provided by different schemas (the exact UI will need discussion). 

This is a weird situation, considering that "set to default" does not write a default value, but removes the user's. We could change the user value to be one of the defaults, but that could break code that rely on default being "true default" (that would probably be just Dconf Editor code, but even then... :). 

> So I’d
> only count two cases: keys are of the same type (including similar ranges),
> or not. In the second case, locking doesn’t look like a bad solution, that’s
> definitively a dangerous case, and something is clearly going wrong. In the
> first case, we should allow the user to read the different
> summaries/descriptions and choose to edit or not.

The more I think about it, the more I believe they should be editable only if both keys are *identical* w.r.t types and values (including defaults and ranges). Otherwise they should be locked-down.

(In reply to Davi from comment #3)
> > The “group by schema” thing allows to properly close the RegistryView
> > problem; but it’s not necessary, if that’s difficult to do. 
> 
> It's not hard to group by schemas. I only mentioned that because as I was
> refactoring for the dynamic model I had to actively add code to stop showing
> keys from multiple schemas in the RegistryView. But even without that, it
> could only show one of multiple conflicting keys in the RegistryInfo,
> because request_path only takes a path. Clicking either key item would
> always show the same one.
> 
> > There can also
> > be an italic summary “At least two schemas provide different summary for
> > this keys” (if the summaries differ), with the disabling of editing
> > possibilities from RegistryView (or only allowing them is keys are
> > completely compatible). Certainly with a warning icon, anyway.
> 
> It seems to me that it is better to not merge keys from different schemas a
> into single item/properties view, irrespective of how similar they are. I
> think it will be much harder to understand the warnings and what is
> happening if we show a single item. I'd rather treat them as individual keys
> everywhere.

I cannot figure what behaviour would be the best. So choose one and we’ll test. ^^

> True, but lock-down should apply to RegistryView too (boolean switches and
> context popovers).

I think I won’t be surprised if a key with a “warning” icon wasn’t editable anyway. So it might be easier to just “lock” duplicated keys from the RegistryView, be they compatible with their clones or not.

> > There could be two “set to default” buttons if there are two defaults
> > provided by different schemas (the exact UI will need discussion). 
> 
> This is a weird situation, considering that "set to default" does not write
> a default value, but removes the user's.

Yes, you’re right, thinking twice that would probably feel bad.

> We could change the user value to be one of the defaults, but that could
> break code that rely on default being "true default" (that would probably
> be just Dconf Editor code, but even then... :).

Let’s try not to lie to the user, the situation of a key could be hard enough to understand without adding a new thing to know about. ^^ A “set to this schema default value” would be acceptable, but not easy to understand, so it’s probably better to lock then.

> The more I think about it, the more I believe they should be editable only
> if both keys are *identical* w.r.t types and values (including defaults and
> ranges). Otherwise they should be locked-down.

Yeps, agree. :)

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

0001 introduces structured descriptors. They contain both path and schema information and will be useful for differentiating conflicting keys. This patch also replaced paths with descriptors in several places, mostly as a consequence of changing DConfWindow.request_path, SettingsModel.get_object and the PathBar.set_path. It does not change Bookmarks and the startup code, so it is not possible (for now) to address conflicting keys reliably from these features. Notice that descriptors moves a little bit more away from SettingObject.

0002 Enables showing multiple schemas. I tried to cover most common cases of conflicting keys, including showing special icons and warnings. Probably many corner cases to handle yet.

Yay, it’ great to see you work on all the problems one after the other, that’s changing from the 2+ years I had working alone on this. ^^’

> 0001 introduces structured descriptors. They contain both path and schema
> information and will be useful for differentiating conflicting keys. This
> patch also replaced paths with descriptors in several places, mostly as a
> consequence of changing DConfWindow.request_path, SettingsModel.get_object
> and the PathBar.set_path.

I have mixed feelings around descriptors. They do the job, it feels clean, that’s great. On the other hand, that’s going back to sharing a GObject where on many places there was a string passed, making notably harder to prepare the long-future goal of splitting the Model in a “DBus library” (as DBus doesn’t know about GObject, it won’t be able to share them).

I was slowly trying until now to move to passing only strings or booleans when possible, or when there was too many related informations to use GVariant (like for the keys description). The interest of these constructions being that they are easily serializable, so DBus-compatible. And the Descriptor could easily be transformed to a GVariant, as it’s mostly non-mutable (properties are construct, and is_non_relocatable_schema/is_relocatable_schema/is_schema_less/etc. could be made construct properties also…).

> It does not change Bookmarks and the startup code, so it is not possible
> (for now) to address conflicting keys reliably from these features. Notice
> that descriptors moves a little bit more away from SettingObject.

That’s breaking the “1 path <=> 1 view” abstraction more hardly than I had in mind, with all the simplicity of this concept. For information, what’s your plan for fixing these features?

Globally, testing the second patch, there’s with the current proposition a need to allow jumping from one key to the conflicting one(s), and that’s making a little more hard in my mind to defend it. I think it would be quite simpler to enforce back “1 path <=> 1 view”, even if we have to add “tabs” in the said view to not present at the same time two different keys. (Yes, I’m mixing a little the underlying model with the UI here, both UI being possibly done with each model. I’m just trying to find the easiest mapping of both.)

> 0002 Enables showing multiple schemas. I tried to cover most common cases of
> conflicting keys, including showing special icons and warnings. Probably
> many corner cases to handle yet.

The two levels of warning are great. Testing a little, I think the “pending changes” icon should be displayed if the key(s) is/are being edited (as there’s not the problem of them taking different values when being reseted, as there’s a “hard conflict” if default values are different; I have the feeling it’s the good behaviour, even if, testing your proposition, we might allow to “change value” but forbid to “reset”; please add anyway a demo for that case also).

I’d also relax a little the warnings, as the problem is mainly solved here. So:
 – no warning on non-conflicting keys (they are not concerned anymore by a potential problem);
 – no stress on RegistryView:
   * a message in the spirit “that’s unusual” instead of “This could lead to problems […] Edit values at your own risk”,
   * no message at all if there’s no conflicting keys (we’re in a different case, quite possible wanted);
 – on editable conflicting keys, no “This could lead to problems. Edit value at your own risk” but something like “Editing it will also change the value of its sister. Please be careful”. Note that having the two summaries and two description under the eyes in that case would be really useful…

Little typos or so:
 * rename “non-conflicting-key” to “a-non-conflicting-key”, for alphabetical sorting;
 * no punctuation in the summaries (convention);
 * I’ve seen an “integet” somewhere.

Many thanks for this work, it’s quite easier to see what are the needs now. I’m not sure I’ll be able to connect in the next days, so have an happy Christmas if you’re celebrating it. :) I’ll come back next week for some days.

(In reply to Arnaud B. from comment #6)
> I have mixed feelings around descriptors. They do the job, it feels clean,
> that’s great. On the other hand, that’s going back to sharing a GObject
> where on many places there was a string passed, making notably harder to
> prepare the long-future goal of splitting the Model in a “DBus library” (as
> DBus doesn’t know about GObject, it won’t be able to share them).
>
> I was slowly trying until now to move to passing only strings or booleans
> when possible, or when there was too many related informations to use
> GVariant (like for the keys description). The interest of these
> constructions being that they are easily serializable, so DBus-compatible.
>
> And the Descriptor could easily be transformed to a GVariant, as it’s mostly
> non-mutable (properties are construct, and
> is_non_relocatable_schema/is_relocatable_schema/is_schema_less/etc. could be
> made construct properties also…).

I was afraid you'd say something like that, so let me defend them a little bit :). And also try to convince you about my future plans about them.

Descriptors are very different from previous SettingObjects, mainly because they contain (almost) exclusively identification information (the relocatable bit is only used to correctly serialize as string), and no other metadata or value-related properties. The other "virtual" properties are only convenience queries. If you'd prefer, descriptors could be made totally opaque, only used as pure handles to query the model. 

The issue with strings is that they are hard to de-serialize :). Whatever information you need from them, you need to either parse them or do a table lookup. Parsing them is the same as having a structure (only clunkier, code-wise), and table lookup seems overkill for the kind of information that is already part of the string.

I almost went with GVariant, but what really made me go with GObject was that only GObjects can be inserted in GListStores. My idea is to remove SettingObjects completely, or at least push them deeper inside the view, i.e. instead of pass them all the way through DConfWindow -> BrowserView -> RegistryView -> Folder/KeyListBoxRow, pass just the descriptor and query the model for extra info directly inside Folder/KeyListBoxRow. In that vision, SettingsModel.get_children would return a GListStore of Descriptor (as a "side-effect", search would likely become much faster). On the other hand, it could just be a GObject wrapper around a GVariant. Or yet, we could also take GListStore away from the model too, making get_children return string[] or GVariant[], and building the list model in the view.

> That’s breaking the “1 path <=> 1 view” abstraction more hardly than I had
> in mind, with all the simplicity of this concept. 

I don't think “1 path <=> 1 view” would work, since there is no “1 path <=> 1 setting”, nor “1 path <=> 1 schema”. On the other hand, I think “1 (schema:)?path <=> 1 view” fits the model and is still simple enough.

> For information, what’s
> your plan for fixing these features?

The start-up code already supports passing schema information. It is just a matter of hooking it up, really. Bookmarks code could be extended to deal with Descriptors too, and that is not too hard. It just needs a reference to the model to know the relocatable flag for stored bookmarks. The serialization would use the "text" format for descriptors, which is already compatible with the path-only format, so no change is needed in the settings.

> Globally, testing the second patch, there’s with the current proposition a
> need to allow jumping from one key to the conflicting one(s), and that’s
> making a little more hard in my mind to defend it. I think it would be quite
> simpler to enforce back “1 path <=> 1 view”, even if we have to add “tabs”
> in the said view to not present at the same time two different keys. (Yes,
> I’m mixing a little the underlying model with the UI here, both UI being
> possibly done with each model. I’m just trying to find the easiest mapping
> of both.)

Tabs are multiple views :). Even then, there's no way to avoid differentiating the keys both in the model and in the view. If we settle on using a string, it would need to be descriptive enough for that (maybe use a "gsettings+dconf" format).

I like the Tabs. But how do think that would interact with the RegistryView? In this patch-set RegistryView make it seem like conflicting keys are different locations. If both will lead to the same composite view, it would break that pattern. Maybe we could add links to the conflicting keys in the infobar?

> The two levels of warning are great. Testing a little, I think the “pending
> changes” icon should be displayed if the key(s) is/are being edited (as
> there’s not the problem of them taking different values when being reseted,
> as there’s a “hard conflict” if default values are different;

Ok.

> I have the
> feeling it’s the good behaviour, even if, testing your proposition, we might
> allow to “change value” but forbid to “reset”; 

I think it is doable, but I think different defaults might mean completely different semantics. Changing the value might mean different things for different keys. I think it would be important to pass the message "you can edit this, but you should also report it to the app developers".

> please add anyway a demo for
> that case also).

Ok.

> I’d also relax a little the warnings, as the problem is mainly solved here.
> So:
>  – no warning on non-conflicting keys (they are not concerned anymore by a
> potential problem);
>  – no stress on RegistryView:
>    * a message in the spirit “that’s unusual” instead of “This could lead to
> problems […] Edit values at your own risk”,
>    * no message at all if there’s no conflicting keys (we’re in a different
> case, quite possible wanted);
>  – on editable conflicting keys, no “This could lead to problems. Edit value
> at your own risk” but something like “Editing it will also change the value
> of its sister. Please be careful”. Note that having the two summaries and
> two description under the eyes in that case would be really useful…

I totally agree. The "multiple schemas" warning could be replaced by some clear indication of what the actual schemas are (like headers in the list, maybe)

> Little typos or so:
>  * rename “non-conflicting-key” to “a-non-conflicting-key”, for alphabetical
> sorting;
>  * no punctuation in the summaries (convention);
>  * I’ve seen an “integet” somewhere.

Ok.

Back. :)

(In reply to Davi from comment #7)
> If you'd prefer, descriptors could be made totally opaque, only used as pure
> handles to query the model.

I’d prefer, but don’t do that. ^^ Passing opaque structures from the model to the UI code and back is never the correct thing to do; what has to be done in that case is to pass an id for this opaque structure (its index in an array, usually). And that’s more or less where your descriptor code is going, dealing with ids and letting the model manage objects, only sharing data when asked.

I’m not opposed to that solution, even if I find it a little over-engineered. Going in that direction impose to be very careful, about changes that could happen between the id generation and the data querying notably. And, I’m not sure, as long as the work is done in the model, that the ids should be the index of a descriptor, instead of just the id of a settingobject… I’m sceptical.

> In that vision, SettingsModel.get_children would return a GListStore of
> Descriptor (as a "side-effect", search would likely become much faster).

That would be an array of ids. Fast, but if you don’t cache in the UI code the content of each id summary, then you have to do many queries to the model to be able to display each line content (value and summary), and if they are going through dbus (long-term plan), then you have a risk that dbus saturates. You don’t have this problem if you just query from beginning a list of things to display.

Honestly, I’d just go with adding a “disambiguation” string in request_path, with the schema in it. (I think request_path could then take as args the path and the object name, with “/” as a disambiguation string for folders, to avoid having to parse the string in the model.) But if you think there’s a need to go in a more complex direction, I’ll wait to until all is done completely.

> I like the Tabs. But how do think that would interact with the RegistryView?
> In this patch-set RegistryView make it seem like conflicting keys are
> different locations. If both will lead to the same composite view, it would
> break that pattern. Maybe we could add links to the conflicting keys in the
> infobar?

Right, there’s something difficult here and what I had in mind is not a good solution. But I would not try to solve this problem now completely, that could be done after; the RegistryView problem is the more important.

> > I have the feeling it’s the good behaviour, even if, testing your
> > proposition, we might allow to “change value” but forbid to “reset”; 
> 
> I think it is doable, but I think different defaults might mean completely
> different semantics. Changing the value might mean different things for
> different keys. I think it would be important to pass the message "you can
> edit this, but you should also report it to the app developers".

You’re right, that might be problematic if the two keys named identically are not used for the same thing, even if the case is unlikely to happen. Let’s not lose time for that.

> The "multiple schemas" warning could be replaced by some clear indication
> of what the actual schemas are (like headers in the list, maybe)

If I understood what you’re talking about, yeps, that’s the goal. ^^ And anyway, just something simple, things should come easily. :)

Happy new year!

I continued thinking about where your solution is going. You could not “just” pass an id to work on, but you also have to pass the information of if the object is a key or a folder, so that:
 * graphically, with the current UI, we could create a row of a different size and render it, without flickering when data is loaded;
 * other implementation could do something completely different with that information, like having a folders list at the left and a keys list at the right (known UI ^^ ).

That’s really looking complex. It would be really better to go a simple(r) way.

Hey, hope you’re ok! :)

I’ve more or less committed all of your second patch in multiple times, using a “context” string being the schema id (for keys with schema). So I close this bug, even if they are some known corner-cases not handled correctly for now (bookmarks, notably).