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

[Eduardo Robles Elvira]

El Lunes, 19 de Febrero de 2007, David Faure escribió:
> Some comments from reading the code:
> KConfig( "closed_tabs" ... )  => will create a closed_tabsrc. This should
> have konqueror in the name somewhere ;) Maybe KConfig
> ("konqueror_closedtabs")?
>
> + if(!sender()->inherits("KonqUndoManager"))
> This breaks if the class is renamed one day, please use !qt_cast<const
> KonqUndoManager *>(sender()). Testing sender() is a bit hackish though, in
> any case.
>
> m_closedTabsListLength is a constant, so it should become a file-static
> imho.

okay so far, this sounds alright to me and I'll fix it.

> +  m_closedTabsConfig->setGroup( "Closed_Tab"  + QString::number(group) );
> Please use KConfigGroup grp( m_closedTabsConfig, "Closed_Tab"  +
> QString::number(group) ), and then use grp.readEntry().
> setGroup is going away very soon.

There's a lot of chat over the blogs, IRC  and everywhere else KDE development 
is present about KConfigGroup, and I also thought this could be a good idea, 
but I didn't use it because konqueror's functions I used taked a KConfig as 
an argument. So, I can make it use a KConfigGroup, but I will need to move 
those functions to the new "standard", KConfigGroup. I bet that you won't 
stop me for doing this =)

> +  friend class KonqMainWindow;
> Can we avoid that? It's the main user of KonqViewManager anyway, so the api
> needed by the mainwindow can just be made public.

Actually yes, that's not needed at all as everything needed is public.

>
> Well done for the integration into konq_undomanager. I have some questions
> about that though: bool KonqUndoManager::undoAvailable() const
>  {
> -    return ( d->m_commands.count() > 0 ) && !d->m_lock;
> +    if(firstClosedTab() > -1)
> +    {
> +       return true;
> +    } else
> +       return ( d->m_commands.count() > 0 ) && !d->m_lock;
>  }
> Why is this change necessary? "Undo is available if there's any command to
> undo" should be general principle; and I see that tab-closing is done as a
> command, as one might expect. But somehow, tab closing seems to be have
> more priority, according to the top of undo(). Why? If I close a tab, then
> move a file, and then I press Undo - shouldn't that undo the file moving,
> instead of the tab closing? I think containsAnyClosedTab and firstClosedTab
> can just go away... (Otherwise, the strange operator== could be removed by
> not using indexOf, but a simple ++i).

Okay this leads to an explanation of this changes. I hope you understand what 
I was trying to achieve and the solution I ended with:

Okay this is what happens: when Konqueror is in KHTML mode, m_lock is always 
off and it's not possible to execute "normal" undo actions. Read "normal" 
as "current", like "File renamed" or "Moved file". And I understand that the 
purpose of m_lock is precisely to block those when we're in a not editable 
mode, like when the active view is showing a webpage.

But now there's a new kind of actions in the undo list: one that 
doesn't "edit" the content managed by the views. It's the "Closed tab" 
action, which is of a different kind. It's an action that should be able to 
be undone (or however it's said :P) in any kind of view.

So in this case, I treat the "closed tab" action as a normal one when the 
current view is editable, but in the non-editable views (like KHTML) we still 
want to be able to reopen all previously closed tabs, so undoAvailable() 
returns true if there's any closed tab, and it only allows to undo one kind 
of actions of the undo stack: "closed tabs".

That's it!

> slotAddClosedURL -> slotAddClosedUrl - new naming style ;)

Oh yeah, my fault sometimes this kind of thing happens hehe thanks fore 
pointing it out.
>
> +  QList<ClosedTabItem*> m_closedTabsList;
> Memory management might be simpler if using QList<ClosedTabItem> instead of
> a pointer. For instance, what happens if I close 11 tabs? Doesn't the
> oldest one get removed from that list, but would still be in the
> KonqUndoManager's list, as a dangling pointer? So in fact we can't store as
> value (libkonq doesn't know that class), but how about making the undo
> manager -own- the ClosedTabItem, and then m_closedTabsList isn't necessary
> at all? Of course the signal openLastClosedTab should ship the
> ClosedTabItem* to konqueror. No need to store that data in both places
> (even as just a pointer).

As suggested by ereslibre, I didn't limit the number of stored tabs because 
they don't take much memory, I only limited the number of them that are shown 
in the drop down menu. So if you close 11 tabs, all of them will be stored in 
m_closedTabsList, but only the last 10 of them will be shown in the "Recently 
closed tabs" menu. That's why I use a list of pointers instead of an array or 
a lis of values: I need to dynamically create and remove eah item with 
new/delete. And also I belive of this there shouldn't be any dangling 
pointer.

It's true is that we could put everything in the undo list so we don't have 
two lists, that's an idea I didn't take into account, and it  will take less 
space in memory, the only problem will be that 
slotClosedTabsListAboutToShow() will then call a function in the undomanager 
that would run through all undoActions and retrieve a list of 
ClosedTabItem*s. I'm not sure if given that we need pointers anyway you still 
think this is desirable. I suppose yes, but I still want you to tell me =)

> Once the konqundomanager api is cleaned up, I would love to see a few unit
> tests ;) But I can also write those in order to check that the
> konqundomanager code works as advertised.

Everyone loves units tests, maybe I can make some of them and learn to use the 
unit test framework along the way.

Thanks for your time,
          Eduardo Robles Elvira.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20070221/e386fce5/attachment.sig>