[WebKit-devel] Zoom actions change proposal (patch)

[Paweł Prażak]

On Sun, May 3, 2009 at 10:41 PM, Urs Wolfer <uwolfer at kde.org> wrote:

> On Sunday 03 May 2009 22:20:28 Paweł Prażak wrote:
> > Some comments:
> > > 0001-Intendation-cleanup:
> > > Note that such a change makres merging with the original file from
> KHTML
> > > harder, but it's okay for me since it improves readabilty.
> >
> > Hmm, I understand. Is it possible to patch KHTML code to change this (I
> > know it's just a detail but I wouldn't want to debug this code ;)
>
> Yeah, that's a nice idea.
>
> > 0001-Zoom-actions-changes:
> > > * Are you sue the bool var m_zoomTextOnly is always initiated?
> >
> > No, I'm not sure, but all of the settings are initialized the same way as
> I
> > see, so I assumed this is the working way. I don't know if this is a
> good,
> > idea but we can move all bit fields to union with an int and init the
> whole
> > union at once. But I'm not sure if it's necessary.
>
> Okay then.
>
> > * "Zoom Normal" is not a very nice text IMHO. What about "Reset zoom"? Or
> >
> > > what
> > > do you suggest?
> >
> > "Reset Zoom" or "Zoom Reset" are good. I didn't gave much thought about
> the
> > name.
> > Maybe "Default Zoom", but reset is probably better. Is there a convention
> > established in KDE?
>
> Not one I'm aware of...  I still like "Reset Zoom" best. Probably ask a
> native
> english speaking person...


Yes, it would be handy, but for the time being lets stick with "Reset Zoom"
:)


>
> >
> > * Just thought about it: is it the usual way that the user sets an option
> >
> > > for
> > > one tab, and it gets saved as a global setting? Probably you could add
> a
> > > new
> > > option in the HTML options, and respect that option? Though the issue
> > > would be
> > > that KHTML would not follow this setting... So not really sure how to
> > > handle
> > > that. Probably your way is the only really working way. Any thoughts,
> > > anyone?
> >
> > FF have global setting as well, so it's not that bad ;) If we make this
> > setting per tab how do we save this? per domain? I don't know how other
> > people but I only use option to set text only zoom when I'm
> testing/writing
> > the code, so it's not a hot feature anyway. ;)
>
> Okay :) Let it as it is now.
> >
> > 0002-Code-cleanup:
> > > This patch makes merging really harder. I also like nice code styling a
> > > lot,
> > > but I'm not sure if it is worth for loosing automatic merging...
> >
> > Yes, now I understand why it looks how it looks. Now I see there is no
> > point in style cleanup without cleanup upstream.
> >
> >  0003-Added-astyle-script-for-kdelibs-style.-Applyed-sctip.patch:
> > > Have you manually reviewed all changes? In some rare cases astyle
> styling
> > > is
> > > not the optimal thing.
> >
> > No I didn't, but good to know that astyle isn't perfect. Anyway, I can
> try
> > to rewrite the script to force styling only on files that have no
> > imporeted/merged code (but I'm not sure if it makes sense).
>
> The only files which need needs to be merged are in kdewebkit/settings.
> Either you or I need to review all astyle changes before committing.


OK, so I'll blacklist kdewebkit/settings/* in the script and review all
astyle changes later. (tomorrow probably)


> Btw, do you have a KDE SVN account? Otherwise I will commit your changes.


Please commit the changes.

Best Regards,
Paweł
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/webkit-devel/attachments/20090503/aadc81a2/attachment.htm