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

[Eduardo Robles Elvira]

Hi people,

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

You undo closed tabs only of the currently active window, or if there is no 
active window, the last window which was active. This way it keeps track of 
active windows with the 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...

Okay, I'll do the (qint64)this, which is the fastest solution.

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

True, I knew that exists but I didn't remember exactly how did it work =).

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

Good idea, I'll do it.

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

About the indentation, I honestly didn't know what to do, I tried my best but 
each file was a bit different. Personally I've always used tabs for that, so 
go figure hehe

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

Uups, ok

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

You got me :D I'll fix it. I think I used this while debugging some time ago

> [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()?

I suppose you mean KonqMainWindow::currentTitle(). I looked for such a 
function, but I didn't find it. In this case currentTitle() wouldn't work if 
we're closing a tab which is not the current one (i.e. right click the tab, 
and select close tab in the menu, or maybe via dbus).

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

Well, I have tried to explain this before but I think I didn't do it very 
well - I'll do my best this time:
The problem here is that with this patch, the meaning of this lock is more 
visible. The lock should be renamed to something more meaningful 
like "filesLock". 

This lock is there so that you don't mess with files when you shouldn't.  For 
example, everytime the current view is a khtmlpart, the lock is activated, 
because it doesn't make sense to undo a renamed file in another tab or window 
if you're currently reading a webpage. That behaviour happens in konqueror 
even in kde3.

So this is important: the lock is a "fileLock". It can also be locked maybe 
when other file operations are being done, I don't know, I don't care too 
much, what I care is the meaning of it.

Now the undo manager handles something new: closed tabs. Closed tabs are 
completly different. They have nothing to do with renaming, removing, moving 
files, etc. To lock the hability of undoing a closed tab doesn't make sense 
here, so the "fileLock" doesn't affect them.

This way, even with the fileLock active and you're browing the dot in you 
konqueror, you can undo the closing of a tab with the blog of aseigo that you 
just closed in a evident mistake ;-)


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

True! I had trouble finding a solution for this problem, and I like yours more 
and it's also quite simple, so I'll do it.

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

Don't worry too much about redoing the code. The important thing is that we 
learn in the process, the final code is good and if we can have fun while 
doing it then it's just perfect :D

Talking about the tab container: I think that's something I added to 
ClosedTabItem in a time when I didn't understand very well how Konqueror 
worked. I've taken a quick look at the patch and it seems that we can get rid 
of it in ClosedTabItem as you said.

Okay, we can move back ClosedTabItem to libkonq but it's not really necesary 
because it's only used as a pointer to some memory, and we never do any 
member function/var call.


I don't have too much time because I have some exams to study and university 
work to do but tomorrow I'll try to get most if not all of this done in the 
previously mentioned subversion repository and then I'll post a message here 
=)

Thanks for your time,
        Eduardo Robles Elvira.
-- 
"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)