Review Request: Adds support for tabs to the comic applet

[Chani]

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


I haven't tried this patch.. just a quick review before I go do other stuff


/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment250>

    so... your config dialog only allows you to configure the comic for the current tab? awww.
    well... I can't think of a better way anyways... maybe that's not such a bad idea...



/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment248>

    why do you need this check? are there other parts of the code messing with your tab lists?



/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment249>

    you put the text in the list, and then you get it out? faster to just use mComicTitle directly here



/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment246>

    aren't you supposed to accept/ignore the event or something?



/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment242>

    o.0
    I actually had to go read up on static to understand this.
    I have bad feelings about this...



/trunk/KDE/kdeplasma-addons/applets/comic/comic.cpp
<http://reviewboard.kde.org/r/251/#comment243>

    what's entfernen? :)



/trunk/KDE/kdeplasma-addons/applets/comic/configwidget.h
<http://reviewboard.kde.org/r/251/#comment244>

    if it's a hack, you probably shouldn't do it. :)



/trunk/KDE/kdeplasma-addons/applets/comic/imagewidget.cpp
<http://reviewboard.kde.org/r/251/#comment247>

    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.


- Chani


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