Review Request 119115: DolphinTabWidget

[Frank Reininghaus]

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


Thanks for your continued efforts to make DolphinMainWindow more manageable.

Looks mostly good, but I did not finish going through the code yet. I have the feeling that you move around more code than is really needed for splitting out DolphinTabWidget, which does not really make reviewing easier, and makes it more likely that possible regressions and opportunities for further improvements are missed (if I'm not mistaken, there is a regression in master already that I have only noticed now, see comments below).


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

    Why is this if-check needed? IMHO, if the active view has not changed, then the signal should not be emitted at all.



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

    Is moving the code in this function really needed right now? Please try to make all patches as small and self-contained as possible.
    
    If there is a good reason for moving this function right now that I have missed, you should comment on that in the review request, please.



dolphin/src/dolphintabpage.cpp
<https://git.reviewboard.kde.org/r/119115/#comment43202>

    I spent some time trying to figure out how this is related to factoring out DolphinTabWidget from the main window.
    
    Could it be that this is actually a fix (or a part of a fix) for a regression caused by one of your earlier patches? I find that the places selector visibility seems to be wrong in current master.
    
    I would prefer if we do not mix unrelated things in one patch, in particular not without commenting on it. Reviewing this is hard enough already (apparently too hard already, considering that I completely missed the regression, and only found it now after noticing this change here), please do not make it even harder if there is no good reason for it.


- Frank Reininghaus


On July 9, 2014, 8:40 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119115/
> -----------------------------------------------------------
> 
> (Updated July 9, 2014, 8:40 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Implemented DolphinTabWidget class to encapsulate the tab handling from DolphinMainWindow.
> 
> Removed the method for text squeezing because the QTabBar can do this automatically.
> 
> The tab sizes are different, any idea how to fix this?
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 7b0210a 
>   dolphin/src/dolphinmainwindow.h 9c7f185 
>   dolphin/src/dolphinmainwindow.cpp 41bd626 
>   dolphin/src/dolphintabpage.h 95c02ed 
>   dolphin/src/dolphintabpage.cpp c8e4263 
>   dolphin/src/dolphintabwidget.h PRE-CREATION 
>   dolphin/src/dolphintabwidget.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/119115/diff/
> 
> 
> Testing
> -------
> 
> Works great so far.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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