[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: [PATCH] New feature: closed tabs trash bin as in Opera
From: Eduardo Robles Elvira <edulix () gmail ! com>
Date: 2007-02-24 10:30:24
Message-ID: 200702241130.24495.edulix () gmail ! com
[Download RAW message or body]
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.
[Attachment #3 (application/pgp-signature)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic