Review Request 118969: DolphinTabBar (QTabBar based)

[Frank Reininghaus]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118969/#review61749
-----------------------------------------------------------


Thanks, looks quite good! I have a few minor comments, see below.


dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42969>

    I think I would move the setMovable(true) and setClosable(true) calls to DolphinTabBar.
    
    The reason is the following: right now, DolphinTabBar can in principle be used with tabsClosable() == false. But in
    
    DolphinTabBar::mousePressEvent(QMouseEvent* event)
    
    we always close the tab on middle click without checking this setting.
    
    Se we should either add a check (which does not make much sense, because we never use the class without closable tabs in Dolphin) or move the setClosable(true) call to DolphinTabBar. And while we are at it, move the setMovable(true) call as well



dolphin/src/dolphintabbar.h
<https://git.reviewboard.kde.org/r/118969/#comment42968>

    Do we need the getter/setter for the delay at all? AFAICS, this class is only ever used with a hardcoded delay of 750 ms, so we could simplify the code by always using that value and removing the two functions completely.
    
    BTW, I see that KTabBar uses a delay of
    
    QApplication::doubleClickInterval() * 2
    
    which is not 750 ms, is it?
    
    I first thought that we should better do just what KTabBar does, but I cannot see a good reason for linking the auto activation delay to the double click interval. Using a hard coded value might indeed make sense then, but this value should then be the value which is now used for users who did not change their default settings, i.e., 800 ms if I'm not mistaken.



dolphin/src/dolphintabbar.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42963>

    'event' is being passed on to the base class function, so it's not unused.



dolphin/src/dolphintabbar.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42964>

    I think that
    
    if (index >= 0 && event->button() == Qt::MiddleButton)
    
    is better. If neither of the two ifs has an 'else' branch, there is no need to nest the two statements.



dolphin/src/dolphintabbar.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42965>

    In contrast to the old code, this will create a menu which does not show the shortcuts for these actions. But probably showing these shortcuts here is not so important that it justifies adding more cross-dependencies between the new classes, so I guess it's OK.



dolphin/src/dolphintabbar.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42966>

    Minor nitpick: I think that a name like "tabCount" or maybe "numberOfTabs" would make it clearer that this not a pointer to a list of tabs or something like that.



dolphin/src/dolphintabbar.cpp
<https://git.reviewboard.kde.org/r/118969/#comment42967>

    Storing the tab which should be activated in a property of the timer, which then has to be casted to an int, looks a bit complex to me.
    
    Why not simply store it in a member variable m_autoActivationTab?


- Frank Reininghaus


On July 4, 2014, 3:51 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118969/
> -----------------------------------------------------------
> 
> (Updated July 4, 2014, 3:51 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Implemented DolphinTabBar class to encapsulate the tab bar handling from DolphinMainWindow.
> 
> DolphinTabBar is based on QTabBar instead of KTabBar (great for frameworks 5) with
> auto activation.
> 
> Currently the patch has more lines added than removed, but with DolphinTabBar + DolphinTabWidget
> we can remove some more lines.
> 
> (hee no more strange slotTestCanDecode signal/slot with call by reference)
> 
> 
> Note that this patch is completely independent from DolphinTabPage patch.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 518c8b7 
>   dolphin/src/dolphinmainwindow.h 7c3bff4 
>   dolphin/src/dolphinmainwindow.cpp d9fe645 
>   dolphin/src/dolphintabbar.h PRE-CREATION 
>   dolphin/src/dolphintabbar.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118969/diff/
> 
> 
> Testing
> -------
> 
> Works fine so far.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140706/57909a8c/attachment.htm>