Review Request 112567: Move the tab-handling away from DolphinMainWindow (Patch-Series)

[Emmanuel Pescosta]

> On Sept. 8, 2013, 10:21 p.m., Frank Reininghaus wrote:
> > Thanks for your efforts to refactor the tab-handling code.
> > 
> > Hm, I'm not quite sure what I and possible other reviewers are supposed to say about this review request. I think that a patch that removes crucial functionality should not be pushed to master. There are quite a few people (including myself) who use the master branch for their daily work, and most of them would not be happy if splitting the view and opening tabs suddenly does not work any more, even if it's just a temporary regression.
> > 
> > I like the idea of splitting huge changes into smaller independent commits, but also the other two patches are a bit hard to comment on, considering that I don't know what the final result is supposed to be, and that I haven't worked much with that code in DolphinMainWindow at all.
> > 
> > Maybe your patch series could be committed to a feature branch? That would enable others to look at all patches in context and to test it. I would really appreciate any help from others here. As I said, I don't know that code well, and going through all this and comparing with the old code will probably take *many* hours.
> > 
> > I think it might also help if you could provide a high-level overview of what your patch series does, what new classes there are, etc. And maybe even more importantly, what is the benefit of the change? To be honest, at the moment it looks to me like you are proposing to turn a lot of code (which has worked well for a couple of years) upside down, and I'm not sure if I understand what the purpose of the change is.
> > 
> > Please try to make the review job a bit easier for everyone else. I cannot really spend more than an hour per day on Dolphin on average, and my motivation to spend at the very least a week, possibly more, of the daily "Dolphin time" figuring out all that stuff myself, seeing the bug reports pile up during that time, and getting no work done myself is rather low if I don't see the "big picture" and I don't know what the purpose of the change is.

Thanks for your feedback.

Yes I agree with you, that this and the other already published feature requests look a little bit weird, but I think it will be more obvious once I have published all changes ;)

> who use the master branch for their daily work
Yes if we commit these changes to master, we have to commit all patches in this patch series at once. 
So a feature branch for these changes is not a bad idea I think.

> I think it might also help if you could provide a high-level overview of what your patch series does, what new classes there are, etc.
Yes I will definitely do this when I have all patches ready/working. I will draw a small dependency graph for the new classes, so that everybody has a big picture of it ;)

The review requests for "DolphinRecentTabsMenu" and "DolphinViewSignalAdapter" are intended to get early feedback on design decisions.

> And maybe even more importantly, what is the benefit of the change?
1. Make DolphinMainWindow much smaller
2. Encapsulate the functionality into different classes like DolphinRecentTabsMenu, DolphinTabPage (Split View handling) and DolphinTabWidget (Tab handling)
3. Port away from KTabBar to QTabBar and QTabWidget (Brings some other benefits like smooth tab changing - fading, keep settings for every tab page like splitter size, ...)
4. Make it easier for other developers to read the code or to change something in the tab/split view handling of Dolphin


All in all: No loss in functionality but easier to understand, extend and maintain.


- Emmanuel


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


On Sept. 6, 2013, 4:48 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112567/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2013, 4:48 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Move the tab-handling away from DolphinMainWindow (Patch-Series)
>     
> Removed all code in DolphinMainWindow which is related to tabbed browsing,
> except of some functions/slots which are required for the current UI, like
> openNewTab(), openInNewTab(), ...
>     
> The tab-handling will be implemented again in another patch.
> 
> Split View and Tabbed Browsing will not work anymore with this patch!
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinmainwindow.h 7da5801 
>   dolphin/src/dolphinmainwindow.cpp 4128cdf 
> 
> Diff: http://git.reviewboard.kde.org/r/112567/diff/
> 
> 
> Testing
> -------
> 
> Split View and Tabbed Browsing doesn't work (is ok)
> 
> Everything else works fine for me (Only one view)
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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