Review Request #118805 - Created June 17, 2014 and submitted
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
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 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).
Review request changed
Pass on two urls instead of "struct ClosedTab" in restoreClosedTab and in rememberClosedTab.
Revision 2 (+177 -105)
Looks good and works fine for me, thanks! Just two little things that might be worth looking at either now or after this commit is in: 1. The comment that says that up to 8 closed tabs are shown in the menu is wrong. It's only 6 because the "Clear" action and the separator are included in the count. 2. I think that removeAction(QAction*) does not actually delete the action. Therefore, we might have a memory leak here. Valgrind would not report it because the actions are still children of the menu and thus deleted when the process ends, but if the user opens and closes lots of tabs, the memory usage might grow slowly, but indefinitely. So I think that we should add delete statements, unless my assumption that removeAction does not delete is wrong.