D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:54298c16f37a: Present error dialog when user tries to 
create directory named . or .. (authored by tmarshall, 
committed by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13805?vs=38050=38725

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall added a comment.


  In D13805#299580 , 
@elvisangelaccio wrote:
  
  > @tmarshall Do you have commit access?
  
  
  I do not.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

elvisangelaccio added a comment.


  @tmarshall Do you have commit access?

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Nathaniel Graham]

ngraham added a comment.


  @elvisangelaccio?

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall updated this revision to Diff 38050.
tmarshall marked 8 inline comments as done.
tmarshall added a comment.


  I've made the requested changes.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13805?vs=36954=38050

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Nathaniel Graham]

ngraham added a comment.


  Thanks for the update!

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall added a comment.


  Sorry for the delay @ngraham . I've been traveling. I'll have these changes 
complete by the end of today.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Nathaniel Graham]

ngraham added a comment.


  @tmarshall, any update on this?

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, firef, andrebarros, 
emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> knewfilemenu.cpp:860
> +// Test to see if the user has given "." or ".." as a name
> +if (name == "." || name == "..") {
> +KGuiItem cancelGuiItem(KStandardGuiItem::cancel());

Please use `QLatin1String` here, which is faster for string comparisons.

> knewfilemenu.cpp:874
> +KMessageBox::createKMessageBox(confirmDialog, buttonBox, 
> QMessageBox::Critical,
> +   i18n("Folder \"%1\" could not be 
> created:\n\"%1\" is reserved for use by the operating system.", name),
> +   QStringList(),

Please use semantic markup here: 
https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

tl;dr use `%1` instead of `\"%1\"` and `` instead of 
`\n`

> knewfilemenu.cpp:876
> +   QStringList(),
> +   "",
> +   nullptr,

Please use `QString()` here

> knewfilemenu.cpp:884
> +confirmDialog->show();
> +m_text = "";
> +return;

`m_text = QString()`, like we already do in `_k_slotAbortDialog()`.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure, elvisangelaccio
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[David Faure]

dfaure added a comment.


  Well, it makes sense for this check to be at the GUI level, the user 
interaction can be better adapted then, as in this patch.
  
  KIO::mkpath("/home/dfaure/tmp/.") should at least not warn or error in any 
way IMHO.
  
  (btw mkdir of such a path, on the command line, always fails: either the tmp 
dir exists and it says "already exists", or it doesn't exist and it can't 
create the parent dir first)

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall added a comment.


  @ngraham
  
  Name: Thomas Marshall
  Email: tmarshall7...@gmail.com

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Christoph Feck]

cfeck added a comment.


  I think the comment can be omitted. It doesn't add information to the code.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure
Cc: cfeck, elvisangelaccio, dfaure, tmarshall, bruns, ngraham, 
kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, firef, 
andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall added a comment.


  @elvisangelaccio The check has to come before the check for 
`name.startsWith('.')` because that's what launches the "The name  starts 
with a ." dialog.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, dfaure, tmarshall, bruns, ngraham, kde-frameworks-devel, 
michaelh, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

elvisangelaccio added subscribers: dfaure, elvisangelaccio.
elvisangelaccio added a comment.


  Hmm, shouldn't this check be at the kio_file level @dfaure ?
  If we put it here, it will still fail when you try to create a folder name 
"." **not** by using the knewfilemenu in dolphin.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham
Cc: elvisangelaccio, dfaure, tmarshall, bruns, ngraham, kde-frameworks-devel, 
michaelh, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Elvis Angelaccio]

elvisangelaccio added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, dfaure, tmarshall, bruns, ngraham, kde-frameworks-devel, 
michaelh, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall added a comment.


  How am I to provide my name and email? Just in a comment?

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham
Cc: tmarshall, bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, 
navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Nice, I think this is looking great. +1 on the latest wording. A lovely first 
patch!
  
  To land it for you, we'll need your full name and email address, so it would 
be great if you could provide that now. In the future, if you submit your patch 
using `arc` (see 
https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches)
 this will happen automatically via the git authorship information, so we won't 
have to bug you.
  
  Any comments from the #frameworks 
 folks?

INLINE COMMENTS

> tmarshall wrote in knewfilemenu.cpp:873
> I get `newfilemenu.cpp:873:74: error: ‘Error’ is not a member of 
> ‘QMessageBox’`
> 
> `QMessageBox::Critical` seems to work though.

Correct: `QMessageBox::Error` does not exist; it's `QMessageBox::Critical`. See 
https://doc.qt.io/qt-5/qmessagebox.html#Icon-enum

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks, ngraham
Cc: tmarshall, bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, 
navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

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

INLINE COMMENTS

> bruns wrote in knewfilemenu.cpp:873
> Should be QMessageBox::Error in this case

I get `newfilemenu.cpp:873:74: error: ‘Error’ is not a member of ‘QMessageBox’`

`QMessageBox::Critical` seems to work though.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks
Cc: tmarshall, bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, 
navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall updated this revision to Diff 36954.
tmarshall marked 2 inline comments as done.
tmarshall added a comment.


  Changed `QMessageBox::Warning` to `QMessageBox::Critical` and updated the 
error message.
  
  New:
  
  F5981803: image.png 

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13805?vs=36952=36954

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: tmarshall, #dolphin, #frameworks
Cc: tmarshall, bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, 
navarromorales, isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> ngraham wrote in knewfilemenu.cpp:874
> This message might be a little bit too terse. How about something that helps 
> explain the reason as well, like this?
> 
> > "" has a special meaning for the operating system and cannot be used 
> > as the name for a folder.

> ... and cannot be used as the name for a folder.

or for anything else.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks
Cc: bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, navarromorales, 
isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> knewfilemenu.cpp:873
> +
> +KMessageBox::createKMessageBox(confirmDialog, buttonBox, 
> QMessageBox::Warning,
> +   i18n("Cannot create directory with name 
> \"%1\"", name),

Should be QMessageBox::Error in this case

> ngraham wrote in knewfilemenu.cpp:874
> This message might be a little bit too terse. How about something that helps 
> explain the reason as well, like this?
> 
> > "" has a special meaning for the operating system and cannot be used 
> > as the name for a folder.

> Folder " could not be created:
>  "" is reserved for use by the operating system.

We should probably use an error message similar to the one returned from the 
KIO job in case something goes wrong.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks
Cc: bruns, ngraham, kde-frameworks-devel, michaelh, spoorun, navarromorales, 
isidorov, firef, andrebarros, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall updated this revision to Diff 36952.
tmarshall added a comment.


  Fixed a type in the comment and improved the wording of the error message.
  
  New:
  
  F5981755: image.png 

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13805?vs=36922=36952

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: tmarshall, #dolphin, #frameworks
Cc: ngraham, kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, 
firef, andrebarros, bruns, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Nathaniel Graham]

ngraham added a reviewer: Frameworks.
ngraham added inline comments.

INLINE COMMENTS

> knewfilemenu.cpp:859
>  } else {
> +// Test to see if the user has give "." or ".." as a name
> +if (name == "." || name == "..") {

has give -> has given

> knewfilemenu.cpp:874
> +KMessageBox::createKMessageBox(confirmDialog, buttonBox, 
> QMessageBox::Warning,
> +   i18n("Cannot create directory with name 
> \"%1\"", name),
> +   QStringList(),

This message might be a little bit too terse. How about something that helps 
explain the reason as well, like this?

> "" has a special meaning for the operating system and cannot be used as 
> the name for a folder.

REPOSITORY
  R241 KIO

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

To: tmarshall, #dolphin, #frameworks
Cc: ngraham, kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, 
firef, andrebarros, bruns, emmanuelp

D13805: Present error dialog when user tries to create directory named "." or ".."

[Thomas Marshall]

tmarshall created this revision.
tmarshall added a reviewer: Dolphin.
tmarshall added a project: Dolphin.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
tmarshall requested review of this revision.

REVISION SUMMARY
  BUG: 387449
  
  In Dolphin, when a user tries to create a folder named "." or "..", they are 
presented with a dialog that informs them that since the folder name they 
entered begins with a ".", the folder they are about to create will be hidden. 
This is, of course, misleading. This patch instead presents them with a similar 
dialog, informing them of the error and asking them for a new name.
  
  Before:
  
  F5976307: image.png 
  
  After:
  
  F5976315: image.png 

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: tmarshall, #dolphin
Cc: kde-frameworks-devel, michaelh, spoorun, navarromorales, isidorov, firef, 
ngraham, andrebarros, bruns, emmanuelp