Review Request 118964: DolphinTabPage

[Frank Reininghaus]

> On July 1, 2014, 9:54 a.m., Emmanuel Pescosta wrote:
> > dolphin/src/dolphinmainwindow.cpp, line 1996
> > <https://git.reviewboard.kde.org/r/118964/diff/1/?file=284874#file284874line1996>
> >
> >     @Frank:
> >     Should we add "bool DolphinTabPage::secondaryViewActive() const"? - Given that we don't have the View enum anymore.
> >     
> >     Comparing pointers is ugly with DolphinTabPage (codewise).

Yes, I think that something like that would be a good idea, thanks! Comparing pointers is not very pretty indeed.

Using a bool for the active view is also similar to the current DolphinMainWindow code, which uses the bool "isPrimaryViewActive" in the ViewTab struct to keep track of the active view. It seems that Peter also came to the conclusion that a bool is the best solution when he wrote that code.

BTW, I think I didn't emphasize enough in my initial reply that this patch will really make DolphinMainWindow much easier to read and maintain already. Nice work!


- Frank


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


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/20140701/79a49354/attachment.htm>