Review Request: Adds support for tabs to the comic applet

[Matthias Fuchs]

> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp, line 471
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2028#file2028line471>
> >
> >     why do you need this check? are there other parts of the code messing with your tab lists?

true, that is indeed too much


> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp, line 706
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2028#file2028line706>
> >
> >     aren't you supposed to accept/ignore the event or something?

that is probably better


> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp, line 752
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2028#file2028line752>
> >
> >     o.0
> >     I actually had to go read up on static to understand this.
> >     I have bad feelings about this...

Though it did not work otherwise.

Here is the scenario:
You have three tabs: *garfield, questionable content, xkcd
You scroll to the next (qc) and then with only a small stop to xkcd. By doing that the qc plugin and the xkcd plugin are loaded, you'll see xkcd and as soon as qc is finished downloading that will be presented even if you are at the xkcd tab as dataUpdated will get called.


> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/configwidget.h, line 66
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2029#file2029line66>
> >
> >     if it's a hack, you probably shouldn't do it. :)

Forgot to remove that comment, as the above.

When working on the code I add a lot of comments to remember parts that should be different or just need some checking. In this case though I came to the conclusion that it is not as bad and faster then the other way -- asking the comic-data-engine for all plugins with their name etc. that are available -- I was thinking of.


> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp, line 841
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2028#file2028line841>
> >
> >     what's entfernen? :)

Remove. :D
Was part of old code that is not needed anymore and wasn't spotted by me while cleaning.


> On 2009-03-08 13:11:10, Chani wrote:
> > /trunk/KDE/kdeplasma-addons/applets/comic/imagewidget.cpp, line 154
> > <http://reviewboard.kde.org/r/251/diff/1/?file=2031#file2031line154>
> >
> >     again, I think you're supposed to accept/ignore events... and I'm concerned as to how hte scrollbar scrolling interacts with the tab-change scrolling. it feels fragile right now - you're doing lots of checks to make sure nothing conflicts, but I think there's a better way.

I do not like that part as well and thought about moving all the scrolling to imagewidget and then emitting a signal, though I thought that this might be slower than what I currently have.
I hope that there is a better way than what I'm currently doing, would makes things easier in the long run.

Btw. thanks for your comments! I really appreciate that.


- Matthias


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


On 2009-03-07 12:00:29, Matthias Fuchs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/251/
> -----------------------------------------------------------
> 
> (Updated 2009-03-07 12:00:29)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Adds tabs to the comic applet, on default the tab bar is not shown.
> 
> Ctrl + Scrolling on the image also changes the tabs, that way you can hide the tab bar but still access the tabs.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeplasma-addons/applets/comic/appearanceSettings.ui 936459 
>   /trunk/KDE/kdeplasma-addons/applets/comic/comic.h 936459 
>   /trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp 936459 
>   /trunk/KDE/kdeplasma-addons/applets/comic/configwidget.h 936459 
>   /trunk/KDE/kdeplasma-addons/applets/comic/configwidget.cpp 936459 
>   /trunk/KDE/kdeplasma-addons/applets/comic/imagewidget.cpp 936459 
> 
> Diff: http://reviewboard.kde.org/r/251/diff
> 
> 
> Testing
> -------
> 
> Works, though sometimes the tab bar disappears, especially when scrolling back. notmart could not reproduce it though, so maybe a local problem.
> 
> 
> Thanks,
> 
> Matthias
> 
>