[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:       David Faure <faure () kde ! org>
Date:       2007-02-19 15:46:16
Message-ID: 200702191646.17177.faure () kde ! org
[Download RAW message or body]

On Monday 19 February 2007, Eduardo Robles Elvira wrote:
> El Sábado, 17 de Febrero de 2007, escribió:
> > The patch is attached to this email, and in [1] there 's a tarball with the
> > icon that my fellow team mate created for the "closedtabs" button. It's a
> > temporal icon that should go inside kdelibs/pics/crystalsvg. If we change
> > to the new KDE icon theme it probably should get a new one, but till then
> > we need it.
> > 
> > Thepatch is agains kdebase trunk, because it touches both libkonq and
> > konqueror. 
> 
> Okey, now I have learned a bit more about KConfig and such things, and now in 
> this new patch there a lekeage (yes! I didn't delete the KTemporaryFiles I 
> created :P), and now instead of using one new file + one  new temporary file 
> per closed tab, I use only one KConfig for all of them. Much more efficient 
> and logical ;-)
> 
> Once more, if you have any comment or suggestion, insight or you just tested 
> it and liked the feature, don't hesitate to reply inside this thread.

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.

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

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


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

slotAddClosedURL -> slotAddClosedUrl - new naming style ;)

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

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.

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


[prev in list] [next in list] [prev in thread] [next in thread]