Review Request 118964: DolphinTabPage

[Arjun Ak]

> On June 29, 2014, 11:36 p.m., Frank Reininghaus wrote:
> > dolphin/src/dolphintabpage.h, line 67
> > <https://git.reviewboard.kde.org/r/118964/diff/1/?file=284875#file284875line67>
> >
> >     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?

Also isnt KUrl deprecated? http://community.kde.org/Frameworks/Porting_Notes#KDECore_Changes


- Arjun


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


On June 27, 2014, 3:19 a.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118964/
> -----------------------------------------------------------
> 
> (Updated June 27, 2014, 3:19 a.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/7398693f/attachment.htm>