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

[Urs Wolfer]

On Sunday 03 May 2009 18:10:43 Paweł Prażak wrote:
> On Sat, May 2, 2009 at 10:25 PM, Urs Wolfer <uwolfer at kde.org> wrote:
> > On Saturday 02 May 2009 17:47:40 Paweł Prażak wrote:
> > > Hi!
> > >
> > > I found some room for improvements in zoom actions.
> > >
> > > Essentially I propose to add the ability to change between text only
> > > and whole page zoom modes (like "Zoom Text Only" option  in FF).
> > >
> > > I attach the patch with proposed changes.
> > >
> > > There are 3 small irrelevant intendation cleanup changes, I hope it's
> > > OK, if not I can clean up the patch and resend.
> > >
> > > I'm little confused about the style, I've tried to use style of the
> > > surrounding code, but I'm not sure if I did it right, sorry about that.
> > > What style should be used in the webkitkde code?
> >
> > Hi Paweł
> >
> > Thanks for your patch! It looks basically good to me. Some comments:
> > #1: Please create two patches: one for the code style fixes, and one for
> > the
> > functional changes.
>
> Done. I've added astyle script as well (and runned in on the code). I'm not
> sure if you want that big changes (>1000 LOC), but to make it simpler to
> merge this commit with some uncommited changes out there, one can apply
> script first and then merge :)
>
> #2: Please make sure things still build with Qt 4.4, since this code shoul
>
> > still build against stable kdelibs. I have not checked, but I'm not sure
> > if you have paid attention to that.
>
> I thought I had but when I've checked it turned out I didn't ;)
> Anyway I've made some further improvements and removed enlarge/shrink
> actions as they are redundant.
>
> About the coding style: basically we use kdelibs coding style (see techbase
>
> > for a documentation, though webkitsettings.{cpp,h} is a file copied from
> > KHTML
> > and thus uses (used) that coding style.. Please use kdelibs coding style
> > for
> > new code.
>
> I've configured astyle to use kdelibs style.

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.

0001-Zoom-actions-changes: 
* Are you sue the bool var m_zoomTextOnly is always initiated?
* "Zoom Normal" is not a very nice text IMHO. What about "Reset zoom"? Or what 
do you suggest?
* 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?

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...

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.

Thanks a lot for your work!

Bye
urs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/webkit-devel/attachments/20090503/948619bc/attachment.sig