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

[David Faure]

On Sunday 16 September 2007, Eduardo Robles Elvira wrote:
> Hi back!
> 
> I'm glad to tell you I'm back. I've been working all the summer but now I 
> think I will have a little more time that I'll use for developing for KDE. 
> I've created a new patch that works with current version of konqueror and 
> lib/konq as of today for this feature.

This is good news, I was waiting for your updated patch before making more changes
to KonqViewManager and KonqUndoManager.

However I still see some problems in this patch. For instance I think it breaks
with multiple windows: the KonqMainWindow constructor does
 ClosedTabItem::setConfig( new KConfig( "konqueror_closedtabs" + QString::number(qHash(this)), KConfig::NoGlobals) );
which changes a *static* member. So all existing closedtabitems (in other windows) are now using a different config...
This can't work. The KConfig object should be a member of KonqMainWindow.
I wish we didn't have to create temp files on disk, but I realize that there is no way currently to create
a KConfig on top of a QByteArray instead of having it create a file...
Maybe we can implement QDataStream-based loading/saving in konqviewmanager, but that would
duplicate the KConfig-based code, so I'm undecided.

+       emit openClosedTab( 0L ); << this is going to break with multiple windows too, no?
They share the same undo manager, and yet only the last tab closed should be reopened...
I think we want to ship the ClosedTabItem and to test in the mainwindow whether this was
ours or not. (Hmm, in fact what's the point of emitting with 0? The slot says if(!closedTab) return; ....)


I'm also still sad to see KonqUndoManager getting tab-specific code, instead of something more generic
(application-defined undo command), is there any chance you could look at that?
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? It seems broken to me. Especially now that you added a way to reopen tabs
without using the undo manager, I think the undo manager should keep normal undo
behavior which is: undo the last operation. So the changes in undoText and the beginning
of undo should be reverted.

I still don't understand the use of m_lock in your konqundomanager code; m_lock is for locking the
command history in all processes while we're undoing a KIO job, which is an asynchronous operation.
I see no relation with undo-close-tab at all... What breaks if you revert KonqUndoManager::undoAvailable()
to its initial code? Surely if there's a tab-closing to undo, d->m_commands isn't empty, so the initial code
seems fine to me. I remember that you mentionned something like "in web browser mode the undo
manager is disabled" -- can you remind me which code does that? It sounds like where the fix should be instead.

I think this code can lead to crashes:
+       KonqCommand::Stack::Iterator it = d->m_commands.begin();
+       for( ; it != d->m_commands.end(); ++it) {
+          if((*it).m_type == KonqUndoManager::CLOSEDTAB && (*it).m_closedTabItem == closedTabItem) {
+            d->m_commands.erase(it); [...]
+          }
+       }
If you call erase, the iterator is then invalid, so ++it is wrong - or it might skip items, well in any case
this is removeClosedTab(ClosedTabItem* closedTabItem) so you should just add a "return" inside the if,
the tabitem can be there only once.

I don't think kdebase is feature-frozen at this point, and in any case you and I have been discussing
this feature for a long time, it should go in for sure. I hope you understand that I would like the
code to be cleaned up before that, though -- I have had too many experiences of broken
code going in and then not being cleaned up or maintained anymore.

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