Review Request: flicking of WebView with ScrollWidget

[Marco Martin]

> On 2010-03-23 17:00:12, Zack Rusin wrote:
> > /trunk/KDE/kdelibs/plasma/widgets/scrollwidget.cpp, line 1179
> > <http://reviewboard.kde.org/r/3368/diff/1/?file=21476#file21476line1179>
> >
> >     Why was this needed? If we'll filter all child events then it will be possible to flick the scrollbars. The check should probably either stay as it is or be changed to
> >     if (!d->widget.data() || i == d->verticalScrollBar || i == d->horizontalScrollBar)
> >         return false;

a temporary solution, since the webview is child of scrollwidget but not the widget itself


> On 2010-03-23 17:00:12, Zack Rusin wrote:
> > /trunk/KDE/kdelibs/plasma/widgets/webview.cpp, line 80
> > <http://reviewboard.kde.org/r/3368/diff/1/?file=21478#file21478line80>
> >
> >     You're the biggest event filter fan in the world :) Why do we need that?

argh, is an old leftover, wasn't intended to be here whoops :)


> On 2010-03-23 17:00:12, Zack Rusin wrote:
> > /trunk/KDE/kdelibs/plasma/widgets/webview.cpp, line 82
> > <http://reviewboard.kde.org/r/3368/diff/1/?file=21478#file21478line82>
> >
> >     I'd probably just create a vertical or horizontal layout, put the d->webView in it and set the layout on the d->scrollingWidget, this way you don't have to worry about setting d->webView geometry. But I'm not 100% sure if we need the scrollWidget at all, wouldn't it be easier to just set ScrollWidget::setWidget(d->webView) directly?

in that way the whole webview would flick instead of the webview contents? (webview could also be resized to the whole contents but this would be terribly inefficient right?)
scrollwidget could also see that the widget is a webview so starting scrolling the webview instead of just moving the widget...


On 2010-03-23 17:00:12, Marco Martin wrote:
> > I think ideally we shouldn't need a dummy scrollingWidget or any event filters. If it doesn't work then, unless I'm missing something here, there's probably a bug/missing feature in the Plasma::ScrollWidget. I could look at it tomorrow night.
> > Either way we should probably test it with the new QtWebKit with the AcceleratedCompositing option on to make sure it works at all.

well, it's kinda working (still did not try with AcceleratedCompositing, is a build time flag on 4.7 right?)
biggest problem at the moment is that the zoomfactor screw things up since dpesn't seem to be any signal to watch it


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3368/#review4626
-----------------------------------------------------------


On 2010-03-23 14:54:52, Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3368/
> -----------------------------------------------------------
> 
> (Updated 2010-03-23 14:54:52)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> after the last modifications by Zack on Scrollwidget, now flicking is managed by ScrollWidget itself, because it's the only way it can safely filter child events..
> and since html elements will be done by qraphicsitems, it will probably be necessary to do the same thing in WebView.
> this is an early attempt, but doesn't seem to work very well. there is a qgraphicswidget with geometry syncronized with the page contents so flicking it makes the page scroll around.
> 
> it's not always correct since is not possible to watch for a change in the zoom level.
> things still missing: disabling everything with setDragToScroll and support for multitouch events.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/widgets/scrollwidget.cpp 1106404 
>   /trunk/KDE/kdelibs/plasma/widgets/webview.h 1106402 
>   /trunk/KDE/kdelibs/plasma/widgets/webview.cpp 1106402 
> 
> Diff: http://reviewboard.kde.org/r/3368/diff
> 
> 
> Testing
> -------
> 
> it seems significantly slower and quite buggy, but i don't see much more ways to do it.
> 
> another way would be generalizing scrollwidget to make it possible to set properties in the webview instead of moving the clipped widget into the parent, but it would make it even more complex.
> 
> another way could be moving everything back in kineticscroll again, make it visible to the classes that are using it (so won't be possible anymore to use it outside of libplasma) and forward by hand to it sceneeventfilter.
> 
> the latter is at the moment the one i'm leaning towards.
> 
> 
> Thanks,
> 
> Marco
> 
>