|
From: Hannes S. <ha...@yl...> - 2012-07-18 20:14:57
|
Hello Raphael!
Raphael Nestler writes:
> I this patch series adds some changes to the browsersettings code. I
> think using a single enum instead of three different booleans is much
> more intuitive.
It probably is, yes.
> To be able to use the give_feedback() function I needed to set focus
> to the webview widget, otherwise it wouldn't show anything. I consider
> this a bug, since it also doesn't show anything when entering an
> invalid setting.
Hm, well, let's say it is an unwanted side-effect. give_feedback()
simply points to echo(). The check there whether the input field is
focused makes perfect sense in my opinion, because otherwise, it might
just overwrite what the user is typing. After confirming a command line,
I agree that feedback should be activated, though.
> The third patch saves the value for boolean settings into the .var field of
> the setting. I need to do this as well for integer settings, but I wanted to
> receive some feedback on the patch in general first. I also thought about
> using
> something like:
> struct {
> ESetingsType type;
> union value {
> char *s;
> int i;
> gboolean b;
> };
> }
> for the settings to not be forced to save every value as a string.
I don't know if you're aware, but for the "per site settings" patch, I
made all settings into strings internally intentionally. I don't like
this much, but this seemed to be the easiest way to avoid the code to be
specific for each individual case. Would be interesting to consolidate
with your approach.
Hannes
|
