Settings dialog changes, config versioning scheme

[Fernando Lemos]

Hello,

I have created my own "fork" (fork sounds kind of harsh, doesn't it) at GitHub:

http://github.com/fernandotcl/arora/tree/master

I intend to use this fork to correct minor annoyances (or "papercuts",
as the Ubuntu devs seem to be calling them nowadays) that affect me,
resulting in a better user experience overall. I also intend to work
on completing some branches (e.g. the top10 branch, if nobody beats me
to it) so that they can be merged into master.

Right now I'm redesigning the settings dialog so that it follows the
KDE HIG, moving things around, trying to make it feel more natural to
both Firefox and KDE users. I also feel that it needs some cleanup, I
believe it never went through a major restructuration like this
because you can spot legacy stuff in some places. My next targets are
the other several dialogs, overall HIG stuff, maybe some work on
improving KDE integration too.

Some times I'm under the impression that some aspects of the settings
dialogs are the way they are because the config file format restricts
the UI. As I clean the UI up, I'll inevitably have to break the config
file format. I would like to know how much of an issue this is. If
this is a big deal, I can add some kind of config file versioning
scheme.

Right now the new settings dialog should mostly work (except for the
custom style sheet settings which I'm going to fix soon), but it's
under "heavy development". I'm open to suggestions and I'm willing to
adapt the code as necessary so that it can be merged into the
repository in the future.

Regards,

[Zsombor]

I'm far from providing authoritative answers, but I believe, it's better if you create separate branches for every different fixes, in this way, one can decide one-by-one, which change to merge, without analyzing the interdependencies, and cherry-picking the nice ones.
 I can't see, why some Human Interface Design cause problems in our settings format :-) I'm seriously believe it's not the case :)
For me, the biggest problem with the settings dialog is that it (mostly) only modifies values in the config file, and through the code base, there were code fragments everywhere which tries to get some specific keys from QSettings. So If you wan't to modify the format of the settings you have to modify code everywhere :(
 So I'dont know what the other - more experienced - devs are thinking, but If you really want to modify the settings file format, I would suggest, that you should introduce a 'Settings' class, which takes care of reading/writing values through QSettings in a type safe manner, with dedicated 'getters/setters' for every flag, and pass this new object everywhere, so in the future, the settings file format is not thightly burned into the application. But it's just my preference :-)
 
BR,
 Zsombor

[Fernando Lemos]

Hah, that's exactly what I was doing last night (haven't pushed to
GitHub yet). I created a SettingsManager (a singleton) and now I'm
gradually replacing direct QSettings calls by SettingsManager calls.
That way we have some nice improvements:

- We currently have to specify the default value for QSettings.value
every time we need to access a config key, this is no longer the case
with SettingsManager
- In SettingsManager it should be relatively pain free to add
versioning capatibilities
- Settings can be "cached"
- The code gets cleaner and shorter

Regarding creating branches for specific fixes, you're right, I'll do
that from now on. I only commited two changes so far (besides the
settings stuff), and one of them is clearly separated from the
settings stuff and the other can be considered part of the settings
changes, so I don't think merging that code back will be a problem.
But you're right, creating branches for specific changes makes more
sense.

Thanks for the feedback.

Regards,

2009/6/22 Zsombor <gzso...@gmail.com>:

[Daniel Albuschat]

2009/6/22 Fernando Lemos <ferna...@gmail.com>:
>
> Hello,
[snip]

> Right now I'm redesigning the settings dialog so that it follows the
> KDE HIG, moving things around, trying to make it feel more natural to
> both Firefox and KDE users. I also feel that it needs some cleanup, I
> believe it never went through a major restructuration like this
> because you can spot legacy stuff in some places. My next targets are
> the other several dialogs, overall HIG stuff, maybe some work on
> improving KDE integration too.
>
> Some times I'm under the impression that some aspects of the settings
> dialogs are the way they are because the config file format restricts
> the UI. As I clean the UI up, I'll inevitably have to break the config
> file format. I would like to know how much of an issue this is. If
> this is a big deal, I can add some kind of config file versioning
> scheme.

Hi Fernando,

first of all, thanks for your work on Arora ;-)

I put some thoughts on settings (both ui and implementation) in my
blog-post here:

http://daniel-albuschat.blogspot.com/2008/05/arora-configuration-system-proposal.html

Maybe there are some ideas that you can re-use.

Regards,

Daniel Albuschat

--
eat(this); // delicious suicide

[Fernando Lemos]

That's interesting, Daniel. Initially I'll probably stick with the
current QDialog-based settings dialog, but the SettingsManager backend
could allow for stuff like that in the future.

I think I'll redo the fork and split the patches in a more fine
grained way. I think I'll break up the changes into 4 different
branches:

a) SettingsManager and config file versioning
b) SettingsDialog cleanup and UI redesign
c) System-wide proxy settings
d) OpenSearch locale fixes

I think that will make merging into upstream easier. Is there anything
else I should do in order to increase the likelihood of a smooth
merging process?

Regards,

2009/6/22 Daniel Albuschat <d.alb...@gmail.com>:

[Daniel Albuschat]

2009/6/22 Fernando Lemos <ferna...@gmail.com>:

>
> That's interesting, Daniel. Initially I'll probably stick with the
> current QDialog-based settings dialog, but the SettingsManager backend
> could allow for stuff like that in the future.

One thing that I see a bit problematic with the current approach:
I'm usually propagating that one function per setting is the best
thing to do, but there's one major shortcoming here: Extensions can
not add functions to a class, which will lead to inconsistent settings
handling in the main source code and the extensions. I don't have a
solution or better idea and don't think that the current approach
"will not work", but while you're at it, you can maybe think about how
settings for extensions will work, too.

[Zsombor]

On Mon, Jun 22, 2009 at 15:12, Fernando Lemos <ferna...@gmail.com> wrote:

Hah, that's exactly what I was doing last night (haven't pushed to
GitHub yet). I created a SettingsManager (a singleton) and now I'm
gradually replacing direct QSettings calls by SettingsManager calls.
That way we have some nice improvements:

Great :) That's happens when great minds meets each other :-)
 

BR,
 Zsombor

[Fernando Lemos]

2009/6/22 Daniel Albuschat <d.alb...@gmail.com>

I think we could use Qt properties in the settings manager exactly for
that, but it needs more thought.

>
> Regards,
>
> Daniel Albuschat
>
> --
> eat(this); // delicious suicide

Regards,

[Zsombor]

If you can write the html/javascript part of it - accessing the settings with a mocked up javascript object - integrating with the rest of the code would be fairly trivial :)

BR,
 Zsombor

[Fernando Lemos]

Ok, so I implemented two options using the SettingsManager, it's in
the settings_manager branch, if anyone's interested. Now it's a matter
of gradually converting these options (there's about 75 of them), and
when that's complete, write the code that will convert the old
configuration format to the new one (which uses Q_ENUMs instead of
ints for combos, for example, which makes life a lot easier for
futture changes to the SettingsDialog).

Any feedback is appreciated, I understand these changes affect the
entire code so I wanted to make sure the devs are ok with such a
change, I'm not proceeding until then.

The changes can be found here:

http://github.com/fernandotcl/arora/tree/settings_manager

It's three commits:

75491fe974da3e0e51022e815b39509bf178ace7
8b9757b50c4fb2c1251fc1baa4321f71b561c6d7
fba406d1daf97348e27aca6bacf082bb382770ed

Regards,

2009/6/23 Zsombor <gzso...@gmail.com>:

[Benjamin Meyer]

Sorry for not jumping on sooner.

Some minor notes

- I noticed that you forked from my account (icefox) rather then Arora
so you got all of my test branches which you might not want. Simply
deleting them is probably faster then re-forking.
- While Arora will strive to integrate with KDE I just want to mention
that one of the Non-Goals of the projects is to become a KDE only
browser (or Gnome, or windows, or os x). We can integrate with KDE
when in KDE, but I have tried not to tie Arora to any desktop or
theme. For example the back and forward buttons are from the style so
Arora fits into kde and gnome, if the icons are incorrect Oxygen is
usually the place that needs fixing. This in no way means Arora is
anti-kde or anything, just that often we will look for solutions in qt/
themes first before hardcoding. The project goals wiki page can be
found here: http://code.google.com/p/arora/wiki/Goals

Took a glance at your repository just now and I found code for your
settings manager, but none for any ui changes :) As this was what
started this thread that can probably be pushed up first. I would
expect this would mostly be a ui file change with possible minor
changes to the settings.cpp. Separating out the patches is always a
good thing. I don't have very many thoughts on the ui for the
settings dialog (mostly I just copied safari) so that could probably
get in pretty quick.

On the other hand I personally dislike having a massive global
settings class with tons of getters and setters. I have seen various
applications that have it and It just feels very inelegant. But at
the same time I realize that in the current state Arora duplicates the
default values in two places which violates the DRY principle and that
is also inelegant. The settings class is a bit of a mess, but I was
hoping to replace it with qautoconfig (the precursor to kconfigxt
which I worked on for kde) which would make it much cleaner and
shorter. One simpler solution then a settings manager with tons of
set/get would be to have a resource file with the default values
bundled with the binary (easy to update/modify) and a single static
function to read default values from it. Then it would not need a
getCookieLegthTimeValue(), but something like getDefaultValue("cookies/
length"). Using a string would scale a lot better.

-Benjamin Meyer

[Daniel Albuschat]

2009/6/24 Benjamin Meyer <b...@meyerhome.net>:
[snip]

> On the other hand I personally dislike having a massive global settings
> class with tons of getters and setters.   I have seen various applications
> that have it and It just feels very inelegant.

I can absolutely understand your point, but when thinking about it,
it's just the right thing to do. And the reason is because it's
typesafe and refactorable.
Think about one setting being removed. Removing a function will make
the compiler cry on every instance that still tries to read or write
that setting. On the other hand, using string literals - while scaling
better - will leave a potential runtime error open that can not easily
be found (of course you can search for a text, but what happens when
you actually don't know which setting still exists and which does
not?). But what's even more difficult is when the type of a setting
changes. Let's say it changes from an integer to an enum, just like
Fernando thankfully did. That's much harder to deal with in terms of
refactoring.

Daniel

[Daniel Albuschat]

Hi Fernando,

2009/6/24 Fernando Lemos <ferna...@gmail.com>:

>
> Ok, so I implemented two options using the SettingsManager, it's in
> the settings_manager branch, if anyone's interested. Now it's a matter
> of gradually converting these options (there's about 75 of them), and
> when that's complete, write the code that will convert the old
> configuration format to the new one (which uses Q_ENUMs instead of
> ints for combos, for example, which makes life a lot easier for
> futture changes to the SettingsDialog).

[snip]

> The changes can be found here:

[snip]

> 8b9757b50c4fb2c1251fc1baa4321f71b561c6d7

I'm really wondering why they use pointers here. I'd do something like this:

SettingsManager &BrowserApplication::settingsManager()
{
static SettingsManager sm;
return sm;
}

Returning a pointer in C++ is a sign that 0 may be returned. Is this
really wanted? Should 0 be returned when no BrowserApplication has
been created?
Or is it ok to create a SettingsManager without a BrowserApplication?
I can see that for other "singletons" this does not happen, which
kinda leaves me with the question why they are static member functions
in BrowserApplication anyways.

Additionally, why is BrowserApplication::settingsManager() declared
inline in the header instead of in the source file like every other
similar function?
I consider this inconsistent and hence it should be avoided.

> fba406d1daf97348e27aca6bacf082bb382770ed

I really like the transitions from combobox-indexes to enums here. I
know how much pain this may become when re-arranging combobox items
means updating the settings files. enums with switch()-statements are
the way to go here, even if it's significantly more code.

Why did you change "General" into "Main", though? I find "General"
much easier to understand than "Main".

In settingsmanager.h, you renamed the SM_IMPLEMENT_TRIVIAL_OPTION_REF
macro, but forgot to rename the undef for that macro, too.

That's it from me.

Daniel

[Fernando Lemos]

Hey,

Thanks for the feedback! Commenting inline:

2009/6/24 Benjamin Meyer <b...@meyerhome.net>:

>
> Sorry for not jumping on sooner.
>

Don't worry, I didn't mean to hurry you up or anything, heh.

> Some minor notes
>
> - I noticed that you forked from my account (icefox) rather then Arora so
> you got all of my test  branches which you might not want. Simply deleting
> them is probably faster then re-forking.

You're probably right. This is the first time I'm using git, I'll
review what you said when I get home, but I think I understand what
you mean.

> - While Arora will strive to integrate with KDE I just want to mention that
> one of the Non-Goals of the projects is to become a KDE only browser (or
> Gnome, or windows, or os x).  We can integrate with KDE when in KDE, but I
> have tried not to tie Arora to any desktop or theme.  For example the back
> and forward buttons are from the style so Arora fits into kde and gnome, if
> the icons are incorrect Oxygen is usually the place that needs fixing.  This
> in no way means Arora is anti-kde or anything, just that often we will look

> for solutions in qt/themes first before hardcoding.  The project goals wiki

> page can be found here: http://code.google.com/p/arora/wiki/Goals

Yea, I understand that. When I mentioned the KDE HIG, it could as well
be the Gnome HIG or any HIG, there just needs to be some sort of
guidelines because we don't follow a pattern. The KDE HIG was actually
the first guidelines I found, so I thought they could be a good
starting point (specially now that Kubuntu wants Arora).

>
> Took a glance at your repository just now and I found code for your settings
> manager, but none for any ui changes :)  As this was what started this
> thread that can probably be pushed up first.  I would expect this would
> mostly be a ui file change with possible minor changes to the settings.cpp.
>  Separating out the patches is always a good thing.  I don't have very many
> thoughts on the ui for the settings dialog (mostly I just copied safari) so
> that could probably get in pretty quick.

Yea, sorry about that, I backed up and removed the old repository so
that I could introduce the changes gradually, The first step, I
thought, would be creating the SettingsManager (I only realized that
after the initial fork, that's why had to recreate the fork).

>
> On the other hand I personally dislike having a massive global settings
> class with tons of getters and setters.   I have seen various applications
> that have it and It just feels very inelegant.  But at the same time I
> realize that in the current state Arora duplicates the default values in two
> places which violates the DRY principle and that is also inelegant.  The
> settings class is a bit of a mess, but I was hoping to replace it with
> qautoconfig (the precursor to kconfigxt which I worked on for kde) which
> would make it much cleaner and shorter.  One simpler solution then a
> settings manager with tons of set/get would be to have a resource file with
> the default values bundled with the binary (easy to update/modify) and a
> single static function to read default values from it.  Then it would not
> need a getCookieLegthTimeValue(), but something like

> getDefaultValue("cookies/length").  Using a string would scale a lot better.

I understand your concerns, but like Daniel said in another email
(which, btw, sums it up pretty well), type-safety brings some
interesting benefits. This centralization means, of course, that in
order to change settings you have to update SettingsManager,
SettingsDialog and then use the updated stuff, but I see some
important benefits (besides type-safety)::

- Since the config is centralized, it's much easier to do config file
versioning, meaning we can break the config any time we need to and be
able to convert old config formats to the current format
- Typos are detected by the compiler
- Type-safety is enforced by the compiler, defauls can be specified only once
- Since we're using Qt-like getters and setters, we can modify the
behavior of these getters and setters if we want anything more complex
than reading from and writing to QSettings for unconventional options
we might have to deal with in the future

I'm open to suggestions on how we can retain these benefits without
losing the benefits you mentioned.

Regards,

[Fernando Lemos]

Hey,

I agree with you, I only implemented it that way to keep it consistent
with the other *Managers. In that specific case, s_settingsManager is
initialized in the ctor, so unless BrowserApplication fails to
construct (in which case Arora would stop running anyways) the pointer
is always valid.

But yea, consistency-aside we should use references for both
SettingsManager and the LanguageManager at least.

Regards,

2009/6/24 Daniel Albuschat <d.alb...@gmail.com>: