-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118964/#review61199
-----------------------------------------------------------
Thanks for working on this. Having a self-contained request for factoring out a single class makes reviewing easier, but going through a patch of this size is still challenging, so I haven't quite finished yet. The patch contains many nice ideas, but I think that some parts can be improved further, see inline comments. For example, one thing that I don't quite understand yet is why you decided to introduce the ViewType enum - I don't quite see how it makes things easier. IMHO,
tabPage->primaryViewContainer()->view()
looks clearer than
tabPage->viewContainer(DolphinTabPage::PrimaryView)->view()
BTW, I appreciate that you tried to keep the changes minimal, but IMHO, if a function in DolphinMainWindow needs to be modified heavily in this patch, then it may make sense to factor out some part of it to a function in DolphinTabPage now, rather than doing it in a future patch. I'll mention these in inline comments.
dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/118964/#comment42629>
Maybe one could factor out the code inside the loop to
DolphinTabPage::markUrlsAsSelected/DolphinTabPage::markUrlsAsCurrent?
dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/118964/#comment42630>
Maybe factor this out to DolphinTabPage::selectedItemsCount()? Or, if this could be confused with the number of selected items in the active view (I'm not sure), totalSelectedItemsCount()?
dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/118964/#comment42631>
Factor out to DolphinTabPage::setPlacesSelectorVisible(bool)?
dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/118964/#comment42632>
Factor out to DolphinTabPage::refreshViews()?
dolphin/src/dolphintabpage.h
<https://git.reviewboard.kde.org/r/118964/#comment42634>
I think that this API might be confusing.
I see that you use the secondaryUrl parameter twice in DolphinMainWindow, but these can be worked around by giving the DolphinTabPage constructor and DolphinMainWindow::openNew(Activated)Tab an optional secondaryUrl argument, right?
To work around the need for the secondaryUrl parameter inside DolphinTabPage itself (you use it in the constructor and in restoreState()), one could factor out the common code from these two functions and create a new function
DolphinTabPage::init(const KUrl& primaryUrl, const KUrl& secondaryUrl = KUrl()).
This would also ensure that the setup of the views is done the same way in both functions. Right now, it seems that a signal-slot connection is missing in DolphinTabPage::restoreState() when I compare it to the constructor, or am I missing something?
dolphin/src/dolphintabpage.cpp
<https://git.reviewboard.kde.org/r/118964/#comment42633>
I needed some time to understand why this works at all. Then I got that there is an implicit cast from the ViewType enum to the int argument of the splitter. IMHO, this is a rather hackish approach.
This is another reason why I think that the ViewType enum does not really help much, and that separater primaryViewContainer()/secondaryViewContainer() functions would be better.
BTW, I think that storing the view containers in member variables, like in the code in master, might be easier than casting the splitter's children.
- Frank Reininghaus
On June 26, 2014, 9:49 p.m., Emmanuel Pescosta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118964/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 9:49 p.m.)
>
>
> Review request for Dolphin.
>
>
> Repository: kde-baseapps
>
>
> Description
> -------
>
> Implemented DolphinTabPage class to encapsulate the split view handling from DolphinMainWindow.
>
> The signal activeViewUrlChanged in DolphinTabPage is currently unused, but we need it later when
> we implement the tab widget and tab bar.
>
> DolphinTabPage has saveState/restoreState which are using a QByteArray instead of the KConfigGroup to
> be more generic. (we can use it in the recent tabs menu in future ;)
>
>
> Diffs
> -----
>
> dolphin/src/CMakeLists.txt 6e4d9f9
> dolphin/src/dolphinmainwindow.h acf60a4
> dolphin/src/dolphinmainwindow.cpp c60951d
> dolphin/src/dolphintabpage.h PRE-CREATION
> dolphin/src/dolphintabpage.cpp PRE-CREATION
> dolphin/src/dolphinviewcontainer.cpp 57452b9
>
> Diff: https://git.reviewboard.kde.org/r/118964/diff/
>
>
> Testing
> -------
>
> Basic testing done.
>
> I'll test some corner cases tonight.
>
>
> Thanks,
>
> Emmanuel Pescosta
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140629/7b53cc31/attachment.htm>