D8091: Added Hide Read Feed option

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added a comment.
This revision now requires changes to proceed.


  Thanks for the patch, I like this feature. Did not test the patch but generally, it looks fairly good. There are mostly some coding-style nitpicks.

INLINE COMMENTS

> akregator.kcfg:9
> +  <entry key="Hide Read Feeds" type="Bool">
> +      <label>Hide feeds with no unread</label>
> +      <whatsthis>Hides feeds with no unread articles</whatsthis>

Missing "articles" at the end?

> actionmanagerimpl.cpp:127
>  
> +void ActionManagerImpl::slotSettingsChanged() {
> +    QAction *a = action(QStringLiteral("feed_hide_read"));

`{` on a new line

> actionmanagerimpl.cpp:130
> +    if (!a) {
> +        qCritical() << "Action not found";
> +        return;

`qCCritical(AKREGATOR_LOG)`

> subscriptionlistmodel.cpp:96
> +{
> +    if (!m_doFilter)
> +        return true;

Use brackets (`{` and `}` even with one-line statements

> subscriptionlistmodel.cpp:104
> +
> +    QVariant v = idx.data(SubscriptionListModel::HasUnreadRole);
> +    if (v.isNull())

Is this recursive? In other words, does this return `true` even if `idx` is a folder with an unread feed? Otherwise you'll need to traverse the hierarchy here in order not to hide those folders.

> subscriptionlistmodel.h:44
> + */
> +class FilterUnreadProxyModel : public QSortFilterProxyModel {
> +    Q_OBJECT

`{` on a new line

> subscriptionlistmodel.h:48
> +
> +    explicit FilterUnreadProxyModel(QObject* parent = 0);
> +

`nullptr` instead of `0`

> subscriptionlistmodel.h:50
> +
> +    inline bool doFilter() const { return m_doFilter; }
> +    inline void setDoFilter(bool v) { m_doFilter = v; invalidateFilter(); }

`{` and `}` on separate lines, ideally un-inline

> subscriptionlistmodel.h:51
> +    inline bool doFilter() const { return m_doFilter; }
> +    inline void setDoFilter(bool v) { m_doFilter = v; invalidateFilter(); }
> +

un-inline

> subscriptionlistmodel.h:54
> +    //reimpl
> +    inline void setSourceModel(QAbstractItemModel *src) { clearCache(); QSortFilterProxyModel::setSourceModel(src); }
> +

- missing `override`
- un-inline this function

> subscriptionlistview.cpp:398
> +    if (!filter) {
> +        qCritical() << "Unable to cast model to FilterUnreadProxyModel*";
> +        return;

`qCCritical(AKREGATOR_LOG)`

REPOSITORY
  R201 Akregator

REVISION DETAIL
  https://phabricator.kde.org/D8091

To: sagara, dvratil
Cc: dvratil, #kde_pim, dvasin, winterz, vkrause, mlaurent, knauss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20171001/87b15b49/attachment.html>