Review Request 128471: [kio] Some fixes for KNewFileMenu.

Fri Jul 22 05:47:44 UTC 2016

> On July 21, 2016, 9:52 p.m., David Faure wrote:
> > Did you check if knewfilemenutest still passes?
> > 
> > I see that it's not testing the properties dialog case, I'm working on that.
> > 
> > (Maybe it would have been easier to split this into 3 different commits & review requests, if the bugs are completely unrelated. Then we could also discuss how to unittest each fix.)

knewfilemenutest passes successfully.

>Maybe it would have been easier to split this into 3 different commits & review requests, if the bugs are completely unrelated.

Yes the three bugs are pretty much unrelated so have created 3 review requests for them.

> On July 21, 2016, 9:52 p.m., David Faure wrote:
> > src/filewidgets/knewfilemenu.cpp, line 497
> > <https://git.reviewboard.kde.org/r/128471/diff/5/?file=472093#file472093line497>
> >
> >     I don't follow. Doesn't removing this connect, break the case of desktop files not from a resource (usingTemplate == false)? Nothing happens anymore when clicking OK, executeStrategy() -- which is where the copying happens -- isn't called anymore.

When clicking OK the signal QDialog::accepted  is also emitted. In KPropertiesDialog::init this signal is connected to KPropertiesDialog::slotOk which, first calls the method for writing the desktop file and then emits signal KPropertiesDialog::propertiesClosed and KPropertiesDialog::applied.
In other words files are written first(if any) and then signals are emitted. This means the desktop file will be created even without any connect statement.

Though KNewFileMenu::executeStrategy handles file operations but because of the statement (which is at the start):
*if (src.isEmpty()) {
  return;
}*
the function returns as m_src is not set.
Also the copying of application desktop file is handled by KPropertiesDialog. So the call to KNewFileMenu::executeStrategy is not needed.

- Chinmoy

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

On July 21, 2016, 4:30 p.m., Chinmoy Ranjan Pradhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128471/
> -----------------------------------------------------------
> 
> (Updated July 21, 2016, 4:30 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> This patch fixes couple of issues with KNewFileMenu.
> 
> Fix 1: When creating a new file, if a file with the default name already exist then a new name is suggested by *KIO::suggestName*. Now this works fine until the scheme of file's url is "file" but in case the scheme is "desktop" (*like when the url in dolphin is set to desktop:/ or "Folder View Settings > Location" is set to desktop:/*) then the check for file's existence fails because *QFile::exists* doesn't understand the "desktop" scheme. So *KIO::suggestName* is not called and no new filename is suggested in case a file with the same filename already exist. To fix this i used *KNewFileMenuPrivate::mostLocalUrl* in *KNewFileFileMenu::executeRealFileorDir* which will resolve "desktop:/" to the user's desktop path. 
> 
> Fix 2: If a user tries to create a new file and proceeds with the default filename then the file created will have no extension. This happens (*inside KNewFileFileMenu::executeRealFileorDir*) because the filename is read from the *Name* section of the desktop file. To fix this, read the URL(the url is in *templatePath*) from the desktop file, determine the file extension from there and append it in the filename.
> 
> Fix 3: Fix for the bug https://bugs.kde.org/show_bug.cgi?id=363673 . The bug occurs because KPropertiesDialog is initialized (*inside KNewFileMenuPrivate::executeOtherDesktopFile*) with path of a resource file. Now KPropertiesDialog only show properties if the file is a local file(i guess). Thats why  clicking on "Link to Application" and then clicking 'ok' shows an error message . To Fix this i used QTemporaryFile to copy the contents of the application template shipped with kio and then initialize KPropertiesDialog with the temporary files' path.
> (I added KNewFileMenuPrivate::mostLocalUrl here to ensure new filename is suggested when inside "desktop:/".)
> (The temporary file created here must be deleted so i also added QFile::remove to *_k_slotOtherDesktopFile and _k_slotAbortDialog* so that whether or not the user creates a Link to Application, the temporary is always deleted)
> 
> 
> Diffs
> -----
> 
>   src/filewidgets/knewfilemenu.cpp 2e613b1 
> 
> Diff: https://git.reviewboard.kde.org/r/128471/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> linktoapplication.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/07/18/d01841f8-08e3-4609-bd82-b54ea446cdf5__linktoapplication.png
> no extension and no new filename
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/07/18/fbd04e13-c2d3-4acc-a4dc-b4a9053e2cb6__newfile.png
> after_patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/07/18/6cbb3f10-9513-478b-a865-9fc791c006d5__linktoapplication_afterpatch.png
> after_patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/07/18/2e9f5fa5-eaaa-4abc-a0ac-f492fb01faa3__newfile_aftepatch.png
> add new device menu
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/07/21/1e115e07-3546-4abe-b913-9e841c21f672__new_addition.png
> 
> 
> Thanks,
> 
> Chinmoy Ranjan Pradhan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160722/42d3aba1/attachment-0001.html>