----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128471/#review97622 -----------------------------------------------------------
Nice fixes! Just some comments. src/filewidgets/knewfilemenu.cpp (line 457) <https://git.reviewboard.kde.org/r/128471/#comment65802> You should call mostLocalUrl only once, it's an expensive operation (KIO Job). src/filewidgets/knewfilemenu.cpp (line 470) <https://git.reviewboard.kde.org/r/128471/#comment65803> You could have kept a copy of the text without .desktop before the append above ;) QString name = text; text.append(...); and using writeEntry("Name", name) here. src/filewidgets/knewfilemenu.cpp (line 471) <https://git.reviewboard.kde.org/r/128471/#comment65808> This line can be removed given that the KDesktopFile destructor will of course sync to disk. src/filewidgets/knewfilemenu.cpp (line 496) <https://git.reviewboard.kde.org/r/128471/#comment65804> You can use mimeTypeForFile(entry.templatePath) to avoid the conversion to QUrl. src/filewidgets/knewfilemenu.cpp (line 506) <https://git.reviewboard.kde.org/r/128471/#comment65805> Same here. (In fact this means these 5 lines about creating the QUrl based on the typed text could be factorized into a helper method) src/filewidgets/knewfilemenu.cpp (line 789) <https://git.reviewboard.kde.org/r/128471/#comment65806> The QFile::exists check is redundant. QFile::remove will do nothing if the file doesn't exist. Coding style: space after "if" Documentation: adjust the docu for _k_slotAbortDialog at the top of the file (it describes the implementation, bad idea, it should rather describe when this is called) src/filewidgets/knewfilemenu.cpp (line 960) <https://git.reviewboard.kde.org/r/128471/#comment65807> Same as in _k_slotAbortDialog - David Faure On July 18, 2016, 4:32 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 18, 2016, 4:32 p.m.) > > > Review request for kdelibs 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 > errormessage.png > > https://git.reviewboard.kde.org/media/uploaded/files/2016/07/18/759ea00e-a65f-4dde-a05a-d297b63c2c1b__errormessage.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 > > > Thanks, > > Chinmoy Ranjan Pradhan > >
