D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-06 Thread Nathaniel Graham 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:673ee2f1115c: Fix failing knewfilemenu test and 
underlying reason for its failure (authored by ngraham).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48820&id=48821

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-06 Thread Nathaniel Graham 
ngraham updated this revision to Diff 48820.
ngraham added a comment.


  Fix rebase barf

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48819&id=48820

BRANCH
  master

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-06 Thread Nathaniel Graham 
ngraham updated this revision to Diff 48819.
ngraham added a comment.


  Rebase on current master

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48818&id=48819

BRANCH
  fix-test-and-code (branched from master)

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp
  src/ioslaves/protocols/mms.protocol
  src/ioslaves/trash/kcmtrash.desktop
  src/urifilters/ikws/searchproviders/nl-teletekst.desktop

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-06 Thread Nathaniel Graham 
ngraham updated this revision to Diff 48818.
ngraham marked an inline comment as done.
ngraham added a comment.


  Minor code fix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48783&id=48818

BRANCH
  fix-test-and-code (branched from master)

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-06 Thread David Faure 
dfaure accepted this revision.
dfaure added inline comments.

INLINE COMMENTS

> knewfilemenutest.cpp:170
> +dialog->accept();
> +path = "";
> +} else {

path.clear() or path = QString() would be better; but ok this is only a 
unittest ;)

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-05 Thread Nathaniel Graham 
ngraham updated this revision to Diff 48783.
ngraham added a comment.


  Test for failure too

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48508&id=48783

BRANCH
  fix-test-and-code (branched from master)

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-05 Thread David Faure 
dfaure added a comment.


  That makes sense. You could set expectedFileName to empty, and change the 
test code to interpret this as "nothing got created (and possibly: check that 
we got an error signal, if there's one)"

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-04 Thread Nathaniel Graham 
ngraham added a comment.


  In D17911#386581 , @dfaure wrote:
  
  > Hmm, let me read the unittest more closely.
  >
  > The scenario is: "New Folder" exists. The user asks to create a new folder, 
the initial suggestion in the dialog is "New Folder (1)". The user removes the 
(1), and hits OK, i.e. the user is asking to create "New Folder" again.
  
  
  This is now an error condition. So maybe the test should check to make sure 
it fails?

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-04 Thread David Faure 
dfaure added a comment.


  Hmm, let me read the unittest more closely.
  
  The scenario is: "New Folder" exists. The user asks to create a new folder, 
the initial suggestion in the dialog is "New Folder (1)". The user removes the 
(1), and hits OK, i.e. the user is asking to create "New Folder" again.
  The unittest is then just checking that "New Folder" exists, which is kind of 
broken since it does exist anyway, since that's the initial setup.
  
  But now you're testing that "New Folder (1)" gets created? That seems really 
odd, it's not what the user is asking for.

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-03 Thread Nathaniel Graham 
ngraham added a comment.


  Tagging is on January 5th, so I'd like to get this in before then. Any 
objections from anyone if I commit this?

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-02 Thread Nathaniel Graham 
ngraham added a comment.


  Thanks! Does the code change in `src/filewidgets/knewfilemenu.cpp` make sense 
to you? I tested a bunch but I know you're the expert here. :)

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-02 Thread David Faure 
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Tests are good :-)

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-01 Thread Nathaniel Graham 
ngraham updated this revision to Diff 48508.
ngraham added a comment.


  Revert unintentional whitespace change

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17911?vs=48507&id=48508

BRANCH
  fix-test-and-code (branched from master)

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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

D17911: Fix failing knewfilemenu test and underlying reason for its failure

2019-01-01 Thread Nathaniel Graham 
ngraham created this revision.
ngraham added reviewers: Frameworks, dfaure, elvisangelaccio, aacid.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  After ef57863a73c0e58b94f9f9f0aa1f4e918d98e79a 
, 
the `knewfilemenu` test started failing. This was for three reasons:
  
  1. I forgot to run the tests, sorry about that!
  2. The test itself had an error that was being masked by the bug fixed in 
that commit
  3. `KIO::mkdir` is a subclass of `SimpleJob`, and is capable of creating 
directorties, so the previous code did not emit the correct signals because the 
SimpleJob condition can result in the job having its `newDirectoryURL` property 
set
  
  This patch fixes the above issues by correcting the bug in the test and 
always checking for the `newDirectoryURL` property no matter what kind of job 
it is.

TEST PLAN
  - All tests now pass
  - Copying files and folders still works

REPOSITORY
  R241 KIO

BRANCH
  fix-test-and-code (branched from master)

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

AFFECTED FILES
  autotests/knewfilemenutest.cpp
  src/filewidgets/knewfilemenu.cpp

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