Review Request 118805: DolphinRecentTabsMenu

[Emmanuel Pescosta]

> On June 18, 2014, 12:54 p.m., Frank Reininghaus wrote:
> > Thanks for the patch - as we've recently discussed in a private conversation, I agree that it's a good idea to push the DolphinMainWindow refactoring forward again, and small self-contained patches that factor individual classes out of DolphinMainWindow are a good way to start.
> > 
> > I'm wondering if DolphinMainWindow needs to know the internals of the ClosedTab struct at all - it would be better for encapsulation if it wouldn't.
> > 
> > I think that the slot "restoreClosedTab" could also just take two URLs as arguments (if the second one was KUrl(), then it would mean that there is only one view in the tab).
> > 
> > Similarly, the signal "rememberClosedTab" could just have the URLs as arguments. Loading the icon via KMimeType::iconNameForUrl(QString&) can easily be moved from DolphinMainWindow::closeTab(int index) to DolphinRecentTabsMenu.
> > 
> > Similarly, squeezing the text could be done in the menu (if it is necessary at all - the "Recently closed tabs" sub-menu is less size-constrained than the tab bar).

> I think that the slot "restoreClosedTab" could also just take two URLs as arguments
Yes, great idea!

In future we can also use the saved state from DolphinTabPage (I'm implementing it right now) 
to save all the information about a tab. Then we can replace the two urls as arguments by
active url and state byte array. - The great thing is, we use the same code for the normal 
session management ;)

> Similarly, the signal "rememberClosedTab"
Yes I'll change it.

Thanks! Will update my patch soon.


- Emmanuel


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


On June 17, 2014, 9:41 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118805/
> -----------------------------------------------------------
> 
> (Updated June 17, 2014, 9:41 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Implemented DolphinRecentTabsMenu to encapsulate the recent
> tabs menu related code from DolphinMainWindow in a new class.
> 
> The DolphinRecentTabsMenu remembers the tab configuration if a
> tab has been closed.
> 
> Branch-Commit: 971391b270a03d9ddb90fddfbabec5cd0a8f1b5c
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 0a72721 
>   dolphin/src/dolphinmainwindow.h cb97612 
>   dolphin/src/dolphinmainwindow.cpp 0ad224c 
>   dolphin/src/dolphinrecenttabsmenu.h PRE-CREATION 
>   dolphin/src/dolphinrecenttabsmenu.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118805/diff/
> 
> 
> Testing
> -------
> 
> Closing a tab -> appears in the recent tabs menu
> 
> Choosing a previously closed tab from recent tabs
> menu an click on it -> closed tab will be opened
> again and is also removed from recent tabs menu
> 
> Closing more than 8 tabs -> oldest closed tabs
> will be removed (FIFO - maximum 8 entries)
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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