D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski marked an inline comment as done.
jglogowski added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kfilewidget.cpp:132
> Nitpick: I'd make `filter` the first parameter. And we usually don't use `const boll` in signatures, but just `bool`.
> 
> `bUpdate` doesn't tell the reader what this variable is used for. How about calling it `updateCurrentFilter` or similar?

Kind of Done.

> dfaure wrote in kfilewidget.cpp:2463
> Is the bUpdate bool necessary?
> 
> Without it, we'd call setCurrentFilter(currentFilter()) which should be a no-op, right?
> 
> Alternatively, the method could return a QString, and the (second) caller could call setCurrentFilter.
> 
> I just don't like a method that's sometimes a getter and sometimes a setter (basically).

> Is the bUpdate bool necessary?



  void KFileFilterCombo::setCurrentFilter(const QString &filter)
  {
      setCurrentIndex(d->m_filters.indexOf(filter));
      emit filterChanged();
  }

filterChanged will unconditionally start the "cycle", which will set the wrong filter.
Wanted to keep my changes more local.

> Alternatively, the method could return a QString, and the (second) caller could call setCurrentFilter.
>  I just don't like a method that's sometimes a getter and sometimes a setter (basically).

Also had that. It "felt" strange, but I don't care much.

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190519/8763b7b7/attachment.html>