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

List:       kde-core-devel
Subject:    Re: enhance KRecentFilesAction with a ui action to remove entries
From:       David Faure <faure () kde ! org>
Date:       2009-03-13 0:23:12
Message-ID: 200903130123.13338.faure () kde ! org
[Download RAW message or body]

On Wednesday 11 March 2009, Christoph Pfister wrote:
> 2009/3/7, Christoph Pfister <christophpfister@gmail.com>:
> > Patch and screenshot attached - comments welcome (don't know how the
> > k-c-d development model works, but I don't mind you / me commiting it
> > if it's ok).
> 
> v2 of the patch:
> - renamed *enable to *visible
> - replaced "Remove all entries" by "Clear list"
> - only show the clear action if the list isn't empty
> - internal: don't delete / reconstruct the special actions, instead
> use setVisible
> - internal: do not put the special actions into selectableActionGroup,
> as the code assumes that those actions are recent actions (e.g. for
> maxItems calculation and in saveEntries)
> 
> Please treat questions like "clear action visible by default" or
> "clear action in general" seperately (the clear action is still
> disabled by default in my patch); if you reach consensus about that it
> can be easily done as a follow-up commit.


+void KRecentFilesAction::setClearActionVisible(bool visible)
+{
+    Q_D(KRecentFilesAction);
+    d->clearActionVisible = visible;
+    visible = visible && !selectableActionGroup()->actions().isEmpty();
+    d->clearSeparator->setVisible(visible);
+    d->clearAction->setVisible(visible);
+}

Do not reuse the boolean "visible", define a new one. It will help readability.

-  virtual void clear();

I'm not sure whether reordering virtuals preserves BC... the comment says
it comes from a base class so I assume so, but, hmm, for extra safety
I would leave it where it is and just add "public Q_SLOTS:" before
(and whatever is needed after like "public:").


Personally I don't see why this feature needs to be optional \
(getter/setter/property... all useless IMHO). I see no reason why the user should be \
prevented from clearing that list. I'm not obeying your request to treat this issue \
separately, because if we agree that it should be on by default and everywhere, then \
the setting isn't necessary anymore. But this isn't a strong opinion, just a \
preference for consistency over pleasing individual application developers.

-- 
David Faure, faure@kde.org, sponsored by Qt Software @ Nokia 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]