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

[David Faure]

On Monday 12 November 2007, Eduardo Robles Elvira wrote:
> 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.

Hmm. I still don't like this :(
It shows a major design problem, where all mainwindows share a konqundomanager
but still the konqundomanager shows different data depending on the active window.
We should really have a layer _above_ the undo manager which shows the right
undo text and enabled status for a given window, and uses the closedtabslist of that window.
I should have thought of that much earlier, it would be a much cleaner design, solving
a lot of the other issues we mentionned - including keeping libkonq clean.
Do you see what I mean? KonqMainWindow talks to, hmm, KonqMainWindow::UndoManager
which has undoAvailable(), undoText(), undoTextChanged() - basically the same API as KonqUndoManager -
and KonqMainWindow::UndoManager knows about
1) the list of closed tabs for this mainwindow
2) the KonqUndoManager instance.
So for instance KonqMainWindow::UndoManager::undoAvailable() would be simply
 return !m_closedTabList.isEmpty() || KonqUndoManager::self()->undoAvailable();
and undoText() {
    if (!m_closedTabList.isEmpty()) 
        return i18n( "Und&o: Close Tab" ); 
    else
        return KonqUndoManager::self()->undoText();
etc.

> 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
Yes I know it's a bit messy, so choose 2 or 4, preferrably 4, but not 1 please :)

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

Well, yeah, OK, but something like currentTitle() does, i.e. calling KonqView::caption().
I don't understand what the two cases are about. We can only close tabs, that's only one kind of frame.
Or is this also about removing split views?

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

OK. This problem, too, would be solved with the above idea of an intermediate UndoManager.
Then it doesn't matter that the underlying file-only KonqUndoManager is locked.

> 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.
OK. I thought the problem was that you were iterating through the whole stack.
I see now that you're not, but the code that confused me was this one:
    t = getClosedTabCommand( m_closedTabsList->first() ).m_type;
which seems useless to me, t will always be set to CLOSEDTAB in this code.


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