[PATCH] New feature: closed tabs trash bin as in Opera

[David Faure]

On Monday 19 February 2007, Eduardo Robles Elvira wrote:
> El Sábado, 17 de Febrero de 2007, escribió:
> > The patch is attached to this email, and in [1] there 's a tarball with the
> > icon that my fellow team mate created for the "closedtabs" button. It's a
> > temporal icon that should go inside kdelibs/pics/crystalsvg. If we change
> > to the new KDE icon theme it probably should get a new one, but till then
> > we need it.
> >
> > Thepatch is agains kdebase trunk, because it touches both libkonq and
> > konqueror. 
> 
> Okey, now I have learned a bit more about KConfig and such things, and now in 
> this new patch there a lekeage (yes! I didn't delete the KTemporaryFiles I 
> created :P), and now instead of using one new file + one  new temporary file 
> per closed tab, I use only one KConfig for all of them. Much more efficient 
> and logical ;-)
> 
> Once more, if you have any comment or suggestion, insight or you just tested 
> it and liked the feature, don't hesitate to reply inside this thread.

Some comments from reading the code:
KConfig( "closed_tabs" ... )  => will create a closed_tabsrc. This should have konqueror in the name somewhere ;)
Maybe KConfig ("konqueror_closedtabs")?

+ if(!sender()->inherits("KonqUndoManager"))
This breaks if the class is renamed one day, please use !qt_cast<const KonqUndoManager *>(sender()).
Testing sender() is a bit hackish though, in any case.

m_closedTabsListLength is a constant, so it should become a file-static imho.

+  m_closedTabsConfig->setGroup( "Closed_Tab"  + QString::number(group) );
Please use KConfigGroup grp( m_closedTabsConfig, "Closed_Tab"  + QString::number(group) ),
and then use grp.readEntry().
setGroup is going away very soon.

+  friend class KonqMainWindow;
Can we avoid that? It's the main user of KonqViewManager anyway, so the api needed by the mainwindow
can just be made public.


Well done for the integration into konq_undomanager. I have some questions about that though:
 bool KonqUndoManager::undoAvailable() const
 {
-    return ( d->m_commands.count() > 0 ) && !d->m_lock;
+    if(firstClosedTab() > -1)
+    {
+       return true;
+    } else
+       return ( d->m_commands.count() > 0 ) && !d->m_lock;
 }
Why is this change necessary? "Undo is available if there's any command to undo" should be general principle;
and I see that tab-closing is done as a command, as one might expect. But somehow, tab closing seems
to be have more priority, according to the top of undo(). Why? If I close a tab, then move a file, and then
I press Undo - shouldn't that undo the file moving, instead of the tab closing?
I think containsAnyClosedTab and firstClosedTab can just go away...
(Otherwise, the strange operator== could be removed by not using indexOf, but a simple ++i).

slotAddClosedURL -> slotAddClosedUrl - new naming style ;)

+  QList<ClosedTabItem*> m_closedTabsList;
Memory management might be simpler if using QList<ClosedTabItem> instead of a pointer.
For instance, what happens if I close 11 tabs? Doesn't the oldest one get removed from that
list, but would still be in the KonqUndoManager's list, as a dangling pointer? So in fact
we can't store as value (libkonq doesn't know that class), but how about making the undo
manager -own- the ClosedTabItem, and then m_closedTabsList isn't necessary at all?
Of course the signal openLastClosedTab should ship the ClosedTabItem* to konqueror.
No need to store that data in both places (even as just a pointer).

Once the konqundomanager api is cleaned up, I would love to see a few unit tests ;)
But I can also write those in order to check that the konqundomanager code works as advertised.

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).