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

[David Faure]

On Sunday 11 November 2007, Eduardo Robles Elvira wrote:
> El Sábado 10 Noviembre 2007, escribió:
> > I've been told we have until tuesday to check in new things in konqueror.
> > So today I've been working in my patch. This way you can get the latest
> > version of the patch:
> >
> > svn checkout https://forja.rediris.es/svn/csl-konqueror
> >
> > I've been posting all the versions of the patch there, so you can get an
> > evolution of it watching the log ;-)
> 
> I've been developing a bit more: now views' history also get restored! This is 
> very useful and it means that you get the history of the view(s) of a tab 
> back when you undo a closed tab, but it's also automagically used when a user 
> restores a whole kde4 session with some konqueror windows back.
> 
> It's all in the undo.patch file of the aforementioned repository =)

+void KonqMainWindow::focusInEvent ( QFocusEvent * )
+{
+       KonqUndoManager::self()->setClosedTabsList(m_closedTabsList);
+}
What does this have to do with focusInEvent?

> It comes with multiple window support working,
Yes it looks better, but it still seems fragile:
+    QString getGroup() const { return "Closed_Tab"  + QString::number(qHash(this)); }
qHash can give you the same value for two different input values.
I think you meant (qint64)this instead.
This all looks a bit dirty but I admit that I don't really see a better solution -- other than
not using a config file at all (which we discussed and found difficult indeed). Well we could
also use one KTempFile per konqueror process and increment a group number for each tabitem, 
I guess I would have done that, but OK...

>+  while ( !m_lstHistory.isEmpty() )
>+    delete m_lstHistory.takeFirst();
Faster as 
qDeleteAll(m_lstHistory);
m_lstHistory.clear();

> config.readEntry( QString::fromLatin1( "m_lstHistorySize" )
Can I suggest better-looking config keys? :-) No m_ or lst prefix, just HistorySize.
Sometimes people do read KConfig files. (and a variable renaming shouldn't break config files, too)

I would put ClosedTabItem into its own file (with a Konq prefix btw) to avoid the two extra
#includes in konq_mainwindow.h -- but that's the kind of cleanup I can do once you commit.
Same for the K_GLOBAL_STATIC that should be used for ClosedTabItem::config so that it's not leaked...
Same for the indentation which should be 4 space (also ok for 2 spaces if the file uses that currently), but not 1.

> +  m_paClosedTabs->setWhatsThis( i18n( "Move backwards one step in the closed tabs history<p>" ) );
Why <p>? No reason for make this string different from the tooltip one if there's no content difference.

[KonqMainWindow::slotOpenLastClosedTab]
+ closedTabItem = m_closedTabsList.first();
+ if(!closedTabItem)
+ {
+   kDebug(1202) << "!!! closedTabItem = 0L" << endl;
+   if(m_closedTabsList.isEmpty())
+     m_paClosedTabs->setEnabled(false);
+    return;
+ }
This code has flawed logic. If the list is empty (as tested on the 5th line) then first() would have asserted....
Test for emptyness first, -then- call first, not the other way round. This seems to be the wrong place
for disabling the action anyway; if the list is empty the action should be disabled already and this
code shouldn't even be needed (ok for a "if empty return", but disabling should be done elsewhere).

[KonqMainWindow::slotAddClosedUrl]
+ if ( frame && frame->activeChildView() )
+ {
+     title = frame->title().trimmed();
+     if ( title.isEmpty() )
+         title = frame->activeChildView()->url().url();
+
+     title = KStringHandler::csqueeze( title, 50 );
+
+     url = frame->activeChildView()->url().url();
+ } else if ( frame2 && frame2->activeChildView() )
+ {
+     // TODO: in case we are a KonqFrameContainer.. how do we get the title of the selected view?
+     title = frame2->activeChildView()->url().url();
+
+     title = KStringHandler::csqueeze( title, 50 );
+
+     url = frame2->activeChildView()->url().url();
+ }
This is really complicated.... What's wrong with currentTitle()?

== Now about libkonq

-    KonqUndoManager::CommandType t = d->m_commands.top().m_type;
+    KonqUndoManager::CommandType t;
+
+    if(d->m_lock && !m_closedTabsList->isEmpty()) {
+       t = getClosedTabCommand( m_closedTabsList->first() ).m_type;
+    } else
+       t = d->m_commands.top().m_type;

This use of d->m_lock still makes no sense to me. Why is it there?
(same thing in a few other places)

In fact the whole change seems wrong to me. From our earlier discussion:
> > From the changes in undo and 
> > undoText, I see that the main reason is that if you close a tab and then
> > you move a file, you want Ctrl+Z to reopen the tab, not to undo the file
> > move. Is this correct? 
> 
> Nop. In that case, Ctrl-Z would move back the file.

So why are you looking into the stack for the last closed tab command, in undo(),
instead of just looking at the top of the stack like the old code did?



-  void undo();
+  void undo(KonqMainWindow* mainWindow);
Eeeeeeeeek. This is really ugly: using an application class from a library that doesn't know this class.
It might compile and work in konqueror, but it breaks any hopes of using that class elsewhere...
which we do - for instance in dolphin. At least use a base class that libkonq can know about, like KMainWindow.
Or better: how about not passing in a mainwindow at all, and changing the openLastClosedTab signal to
pass the ClosedTabItem*? Then in KonqMainWindow we could just do
void KonqMainWindow::slotOpenLastClosedTab(ClosedTabItem* tabItem)
{
    if (!m_closedTabsList.isEmpty() && m_closedTabsList.first() == tabItem) {
        slotOpenLastClosedTab();
    }
}

This way, only the mainwindow that had this tab, will reopen it, with less changes to KonqUndoManager
and no notion of mainwindow in there.

Hmm, but it's the same problem with ClosedTabItem; it's defined in konqueror and used in libkonq... :(
How about removing the tabContainer member (there is only one tabContainer per window anyway,
you can use m_tabContainer instead of closedTab->tabContainer) and moving that class to libkonq?
I know it was there initially and I said it would be better to have (all) that code in konqueror but as 
long as konq_undo uses ClosedTabItem, that class should be in libkonq.

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