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

[Eduardo Robles Elvira]

El Lunes, 19 de Febrero de 2007, David Faure escribió:
> KConfig( "closed_tabs" ... )  => will create a closed_tabsrc. This should
> have konqueror in the name somewhere ;) Maybe KConfig
> ("konqueror_closedtabs")?

Fixed

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

Fixed, now it doesn't test the sender anymore

> m_closedTabsListLength is a constant, so it should become a file-static
> imho.

Done

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

Okey this is the only thing I didn't fix. Using a KConfigGroup required major 
changes in konqueror code, probably including plugins and such. Once 
KonqFrameBase::saveConfig and KonqFrameBase::loadItem end up taking a 
KConfigGroup instead of a KConfig as an argument, I promise I will stop using 
setGroup.

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

Done, friend statement removed

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

As I said in the last email, this is necessary. But operator== have been 
removed ;-)

> slotAddClosedURL -> slotAddClosedUrl - new naming style ;)

Done

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

Done (probably it was the most difficult part ;-)
Now there's no m_closedTabsList anymore, every closedTab is only listed in the 
UndoManager.

Probably you have still some questions, if so I will answer and try to correct 
any problem with the code.

        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/20070224/4266fbf9/attachment.sig>