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

[Eduardo Robles Elvira]

Hello!

I'm at work so forgive me if this email doesn't answer with enough details:

El Miércoles, 19 de Septiembre de 2007, escribió:
> 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.

I completly forgot the multiple windows problem. I'll find a good fix and have 
it in the next patch I'm developing. Stay tunned =).

I think we should to stick to using kconfig, if I recall correctly there was a 
way (something related to not calling sync() and a Kconfig flag) that allowed 
us to not touch the disk. And of course a very good reason is that we don't 
want to duplicate code: this kconfig code is already being used and will be 
used also in another patch to support session management like opera/firefox 
in the future.

> +       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;
> ....)

Yes, this will break with multiple windows. I will study the best solution 
amog different possiblities, but ClosedTabItems will store which window it 
came from, for sure.

The signal is:
	void openClosedTab( ClosedTabItem* closedTab );

IT might be confusing and probably I will change it, but the current meaning 
for this is : if closedTab == 0L, then there's no reference given to the tab 
closed. What to do? Just pop the last tab on the "stack".

> 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?

Maybe it could get more generic, using some kind of general pointer. I will 
study this option. This could be interested because I have been thinking of 
improving the patch to be able to recover.. not only tabs, but windows. 
Like... the last opera version hehe. Probably this feature should change it's 
name, from "closed tabs" to "recover" or "recycle bin" (be it recover closed 
tabs or closed windows) or something similar.

> 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.

> 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.

Right ;-) That's what it is supposed to do.

> 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 don't remember exactly where it happened, and I will debug this to be sure, 
but that's what happens: "in web browser mode the undo manager is disabled". 

As I see it, m_lock was thought to block file operations. That's probably why 
in web browser mode m_lock is locked, because it doesn't make sense to undo a 
file move in web browser mode. I think that behaviour is correct. Also, I can 
imagine the m_lock being blocked for async file operations as you mention.

But closed tabs are not "file oeprations", and that's why m_lock should affect 
it. So the behaviour is: if m_lock is disabled, you just undo last operation. 
But if it's locked, all file operations should be locked, but closed tabs 
(and other not file-related ops that you could imagine, if any) shouldn't be 
locked, at least not with m_loc, which probably should be renamed to 
m_file_lock.

> 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 will take a look at this :P

> 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.

I'm glad to read that! And I understand and share your position. We want to 
keep konqueror in good shape!

Stay tunned,
         Eduardo Robles Elvira (Tecnova).
-- 
"The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all progress
depends on the unreasonable man." (George Bernard Shaw)