Review Request 120144: Equal entries imply existing

[Jan-Marek Glogowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120144/
---

Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

Equal entries imply exist


Diffs
-

  src/ioslaves/file/file_unix.cpp bd16ce33e188b6613b3fdc8d712095144808bf94 

Diff: https://git.reviewboard.kde.org/r/120144/diff/


Testing
---

The equal file error should take precedence over the existing directory error, 
as this implies exist.

This actually prevents a data loss on case-insensitive file systems like CIFS, 
where renaming a directory from 'Test' to 'test' actually removes the single 
directory (both have the same inode).

This was just tested and fixed in our KDE4 kfile handler and konqueror, which 
have the same problem (KDE 4.12 on Kubuntu 12.04).


Thanks,

Jan-Marek Glogowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120144: Equal entries imply existing

[Jan-Marek Glogowski]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120144/
---

(Updated Sept. 12, 2014, 1:07 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Repository: kio


Description
---

Equal entries imply exist


Diffs
-

  src/ioslaves/file/file_unix.cpp bd16ce33e188b6613b3fdc8d712095144808bf94 

Diff: https://git.reviewboard.kde.org/r/120144/diff/


Testing
---

The equal file error should take precedence over the existing directory error, 
as this implies exist.

This actually prevents a data loss on case-insensitive file systems like CIFS, 
where renaming a directory from 'Test' to 'test' actually removes the single 
directory (both have the same inode).

This was just tested and fixed in our KDE4 kfile handler and konqueror, which 
have the same problem (KDE 4.12 on Kubuntu 12.04).


Thanks,

Jan-Marek Glogowski

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
jglogowski requested review of this revision.

REVISION SUMMARY
  If KFileWidget's filter list has two matching filters for an extension,
  it will always select the first filter, even if the current filter
  already matches the file name.
  

  
  This is fine, if you auto-select the filter to match the file name, but 
  breaks, if you want to auto-change the file name's extension via the 
  selected filter.
  

  
  So this checks, if the current filter already matches the file name
  before trying to find a matching filter and select it.
  

  
  BUG: 407642

TEST PLAN
  1. Compile and run the attached program to the bug report 407642
  2. Make sure that "auto extension" checkbox is enable
  3. Select the last file filter (DocBook (.xml)) via dropdown list
  
  OBSERVED RESULT
  Filter is "Word 2003 XML (.xml)"
  
  EXPECTED RESULT
  Filter is "DocBook (.xml)"

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jglogowski
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58193.
jglogowski added a comment.


  Readd dropped QLatin1Char and use them in new code too.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58191=58193

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jglogowski
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski planned changes to this revision.
jglogowski added a comment.


  This patch was developed on KIO v5.44 (git tag) in an Ubuntu 18.04 chroot 
(because that's my LibreOffice development environment) and rebased on master.
  The test-program was run on Debian Buster via 
LD_PRELOAD=./git_kio/build/bin/libKF5KIOFileWidgets.so, which has otherwise KIO 
v5.54.
  
  I tried to build master on Ubuntu 18.04, but that failed. The git log for 
kfilewidget.cpp looked unsuspicious enough.
  
  There were some conflicts in the rebase, which I manually fixed, but as 
always, this might result in typos, as I couldn't even compile master.

REPOSITORY
  R241 KIO

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

To: jglogowski
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski added a reviewer: Frameworks.

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks
Cc: michaelweghorn, kde-frameworks-devel, michaelh, ngraham, bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58215.
jglogowski added a comment.


  Hope this compiles now. Since I can't test the rebased patch, I hope that is 
the last update.
  The original working version is based on v5.44. See my previous comment.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58193=58215

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jglogowski, #frameworks, ngraham
Cc: ngraham, michaelweghorn, kde-frameworks-devel, michaelh, bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58317.
jglogowski added a comment.


  Readd dropped lines from tests/CMakeLists.txt

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58316=58317

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp
  tests/CMakeLists.txt
  tests/kfilewidgettest_filter.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58316.
jglogowski added a comment.


  - Return a QString from findMatchingFilter and handle setCurrentFilter in 
main function
  - Add filter unit test
  
  For a manual test I added a "Raw (*)" filter to the bug test program.
  After playing with it, I decided to not test the currentFilter() against 
QLatin1String("*"), so if a user selects such an entry, it'll disable 
auto-filter and -extension selection.
  Since we never auto-select the "*" filter, this honors the users filter 
selection.
  
  Took me a while to write the unit test without QDialog. I was a bit tricked 
by Q_ASSERT used in other tests ;-)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58291=58316

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp
  tests/CMakeLists.txt
  tests/kfilewidgettest_filter.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski added a comment.


  In D21249#467037 , @dfaure wrote:
  
  > A unittest addition would be good, too.
  
  
  Done
  
  In D21249#467073 , @dfaure wrote:
  
  > Yes, naming is hard because the method is dual-purposed ;-)
  >
  > If the method only tried to match (but not to set), then the naming would 
be much simpler.
  
  
  …
  Done
  
  > 
  > 
  >> It "felt" strange, but I don't care much.
  > 
  > I do care, because others will try to understand and possibly modify this 
code later, so it should not "feel strange".
  
  I should have written. "Fine with me.". No offense intended.

INLINE COMMENTS

> dfaure wrote in kfilewidget.cpp:2495
> You wrote "done", but it's still there.

Should have been - will do.

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58291.
jglogowski added a comment.


  Changes:
  
  - Dropped the duplicate comment in matchFilter (not sure if it makes sense at 
all)
  - Replace bool param with enum class to improve readability - should have 
done this from the start
  - Drop const from enum as requested; I like useing const wherever possible 
setting preconditions…
  
  Remarks:
  
  - Wondering why there is still this foreach... maybe was just missed
  - Naming is hard and I couldn't come up with something I really liked 
(MatchPoliy vs MatchAction etc.)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58215=58291

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

AFFECTED FILES
  src/filewidgets/kfilewidget.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

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

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58345.
jglogowski added a comment.


  - Merge test/kfilewidgettest_filter.cpp into autotests/kfilewidgettest.cpp
  - Swap QCOMPARE parameters to match actual + expected output on failure
  - Always test filter and file name
  
  Technically the '*' filter just makes sense as the last filter in the list 
and it's auto-selected if it's the first filter entry.
  That's out of scope here. Not sure if there should / could be a warning for a 
developer.
  
  Thanks for your patience.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58317=58345

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski added inline comments.

INLINE COMMENTS

> dfaure wrote in kfilewidgettest.cpp:88
> ... use setUrls *to* auto-select ODT filter?
> 
> ("to" is missing)

No. The first line is missing a point or semicolon, if you read the comment 
like a sentence. I had two independent comments in mind. Or some kind of 
annotation (*) to make it look more like a list.

The first line is a general comment, the 2nd describes the actual test case.. I 
know the blocking is documented, but it's still not expected behavior. At least 
I was puzzled and even read the code before the API doc, as I suspected a bug 
somewhere (that didn't exclude my code).

> kfilewidgettest.cpp:94
> +// select 2nd duplicate XML filter
> +// see bug 407642
> +fw.filterWidget()->setCurrentFilter("*.xml *.b|DocBook (.xml)");

From your POV this probably has the same problem then the first comment. I'll 
change it to:

// select 2nd duplicate XML filter (see bug 407642)

> dfaure wrote in kfilewidget.cpp:2456
> This method could (and should) be marked as const, now.

Will do.

> dfaure wrote in kfilewidget.cpp:2492
> I don't really understand what this comment is doing here, it seems unrelated 
> to the next line of code. Is it just a longer version of the comment on line 
> 2499?

Kind of a reverse comment of line 2499. My original code tested for '*' and I 
found it none-obvious to omit that match test here. I even tested both variants 
with the bug program to be sure. The comment is a little disturbing, as there 
wasn't any code handling '*' yet. I'll change it to:

// accept any match to honor the user's selection; see later code handling the 
"*" match

Or I drop it. I would keep it. Other suggestions?

REPOSITORY
  R241 KIO

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

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58394.
jglogowski added a comment.


  - const findMatchingFilter
  - amend comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58345=58394

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns

D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski updated this revision to Diff 58427.
jglogowski added a comment.


  - switch declaration KFileWidgetPrivate::findMatchingFilter from bool to 
QString … again
  
  Not sure how that exactly slipped in :-(

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21249?vs=58394=58427

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

AFFECTED FILES
  autotests/kfilewidgettest.cpp
  src/filewidgets/kfilewidget.cpp

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, 
bruns