[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:       Christoph Pfister <christophpfister () gmail ! com>
Date:       2009-03-13 12:30:04
Message-ID: 19a3b7a80903130530r16bf7679w8f7c8d639a398772 () mail ! gmail ! com
[Download RAW message or body]

2009/3/13 David Faure <faure@kde.org>:
> 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.

Ok.

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

Yes, this is the easiest and safest way to assure bc - considering
(I've just found out) that the base class "clear" actually isn't
virtual (!).

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

So ... anybody against the clear action being always shown (on
condition that the list isn't empty, of course)?

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

Thanks,

Christoph


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