From kde-core-devel  Fri Mar 13 12:30:04 2009
From: Christoph Pfister <christophpfister () gmail ! com>
Date: Fri, 13 Mar 2009 12:30:04 +0000
To: kde-core-devel
Subject: Re: enhance KRecentFilesAction with a ui action to remove entries
Message-Id: <19a3b7a80903130530r16bf7679w8f7c8d639a398772 () mail ! gmail ! com>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=123694746417610

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