Review Request 110340: Factored out the tabbing code from DolphinMainWindow into different classes + Implemented tab-bar drag and drop (with drop indicator)

[Frank Reininghaus]

> On May 10, 2013, 9:12 a.m., Frank Reininghaus wrote:
> >
> 
> Emmanuel Pescosta wrote:
>     Empty comment? :)

Hm, strange - it looks like I somehow hit the wrong key(s) when submitting my comment and deleted it accidentally. Anyway, here is my comment:

Efforts to split DolphinMainWindow into smaller independent units are certainly welcome. However, I see that those units still depend strongly on each other in both directions, i.e., there are circular dependencies. For example, DolphinTabWidget is accessed by DolphinMainWindow, but it also contains a member m_mainWindow. There are more examples for this pattern (e.g., DolphinTabPage/DolphinTabWidget).

Refactoring code is more than moving the code out of one file and re-arranging it in different files and classes. The structure should be such that these classes are as independent as possible and do not have circular dependencies. Otherwise, the 'refactoring' does not have a real benefit from my point of view.

Moreover, 'friend' and 'reinterpret_cast' don't look like things which make the structure clearer or future maintenance easier either.


- Frank


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


On May 8, 2013, 10:41 a.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110340/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 10:41 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> * Implemented tab-bar drag and drop (with drop indicator) - When you drag folders to the empty tab-bar area, a drop indicator appears, 
>   when you drop the folders there, all folders will be opened in new tabs.
> 
> * Factored out the tabbing code from DolphinMainWindow into different classes (This is smth. Peter had already in mind some time ago). 
>   It reduces the LOC in DolphinMainWindow and should make the maintenance easier in the future.
> 
> * Use the the power of QTabWidget + tab pages instead of just hiding/showing the DolphinViews in the layout (No flickering on tab change anymore)
> 
> New classes:
> * DolphinTabPage - handles the two views + splitter stuff + layout stuff -> no need of hiding/showing view widgets anymore ;)
> * DolphinTabBar - handles drag and drop events + context menu requests on the tab bar. (QTabBar based instead of KTabBar)
> * DolphinTabWidget - based on QTabWidget, does the view handling (connector between all tab pages + tab bar)
> 
> The DolphinTabWidget is the new central widget for DolphinMainWindow.
> 
> To all Dolphin/KDE devs out there, please help me to find all the regressions/bugs caused by this patch and please review this code. Thanks! :)
> 
> 
> This addresses bug 216433.
>     http://bugs.kde.org/show_bug.cgi?id=216433
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt ffb232c 
>   dolphin/src/dolphinmainwindow.h 7da5801 
>   dolphin/src/dolphinmainwindow.cpp 347489d 
>   dolphin/src/dolphintabbar.h PRE-CREATION 
>   dolphin/src/dolphintabbar.cpp PRE-CREATION 
>   dolphin/src/dolphintabpage.h PRE-CREATION 
>   dolphin/src/dolphintabpage.cpp PRE-CREATION 
>   dolphin/src/dolphintabwidget.h PRE-CREATION 
>   dolphin/src/dolphintabwidget.cpp PRE-CREATION 
>   dolphin/src/dolphinviewcontainer.cpp de1ae4b 
> 
> Diff: http://git.reviewboard.kde.org/r/110340/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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