Renaming a file in browse mode preselects the filename without extension.
In view mode the whole filename (including extension) is preselected.
This patch adds a custom dialog, which selects only the filename in view mode rename.
Details
Details
- Open image in view mode
- Press F2 to rename file
-> Filename without extension should be selected
Diff Detail
Diff Detail
- Repository
- R260 Gwenview
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
There are two old bug reports concerning this issue:
https://bugs.kde.org/show_bug.cgi?id=130408
https://bugs.kde.org/show_bug.cgi?id=233361
Seems it was fixed at that time by introducing InputDialog in gwenview v1.
I don't know what happened on the way to v2.
(btw. Happy New Year :) )
Comment Actions
Happy new year, everyone ;)
@muhlenpfordt Looks like you keep submitting patches, and I'm getting tired of always having to commit them for you ;) Have you considered applying for a developer account?
As for the review: I'm still a bit backlogged, sorry it might take a while…
Comment Actions
The patch works fine for me (yay!) but do we know why app/renamedialog.* got removed? Does it seem worth it to re-introduce this functionality without adding more source files?
Comment Actions
It seems, renaming was not ported from v1. It was re-implemented for view mode in 9'2009 (gwenview v4) using standard KInputDialog::getText() acc9dcf231d7
For thumbnail view the special ItemEditor was introduced in 7'2010, which selects the filename only 1b5895a308a3
Does it seem worth it to re-introduce this functionality without adding more source files?
Since QInputDialog doesn't give access to the input line widget, I think the only way to set the selection is a custom dialog.
Or did your question aim to integrate the class into another file?
Not yet - you're much more familiar with this... ;)
Ok, I'll think about it.
Comment Actions
Not really ;) My first patch was https://phabricator.kde.org/D5749, then I got asked in https://phabricator.kde.org/D7163#132997.
Also, reviewing of Diffs would still continue, of course. The application process is mostly about making sure you have a continued interest to contribute in the future, and that you are not a troll breaking the repo…
Let me know if you have any questions. If you want to wait a bit before making the jump, that's totally fine, too.
| app/renamedialog.h | ||
|---|---|---|
| 5 | BTW, you should update that… | |
Comment Actions
I've gone through the documents for a dev account :) and read that the mail address should match that on identity/bugs.kde.org.
My phabricator mail is an old ugly gmx alias, which I want to get rid of - but found no way to change it here? On both other systems, a new address is active.
Is there a trick or do I need a new account?
Comment Actions
Seems fine functionality-wise and makes for a nice improvement in usability.
Before I review the code: Did you look at how Dolphin's code regarding this looks? Dolphin offers the same functionality when disabling inline renaming in the settings.
| app/fileoperations.cpp | ||
|---|---|---|
| 227–233 | Making dialog on the stack that blocks in exec is not good idea at all. If parent gets destroyed in blocking exec function it will destroy dialog itself, exec will return and dialog destructor will be called twice. I saw other functions in this file are written as is but they are wrong too. QPointer<RenameDialog> dialog = new RenameDialog(parent);
...
if (dialog->exec()) {
...
}
delete dialog; | |
Comment Actions
I saw other functions in this file are written as is but they are wrong too.
@anthonyfieroni Thanks for the tip :) Would you mind submitting a new Diff fixing things everywhere you think it's necessary?
| app/fileoperations.cpp | ||
|---|---|---|
| 227–233 | Also, why not QScopedPointer? | |
| app/fileoperations.cpp | ||
|---|---|---|
| 227–233 | It cannot, QScopedPointer holds a raw pointer and it does not track its lifetime. | |
Comment Actions
I didn't know that dolphin offers a rename dialog (I'm rarly using dolphin - I'm the krusader fan, coming from stone aged norton commander... ;) )
Surprise - dolphin uses a custom RenameDialog. I think it's a bit oversized for gwenview, providing support for multiple file renaming. The filename split is done by QMimeDatabase, too.
| app/fileoperations.cpp | ||
|---|---|---|
| 227–233 | Thanks for this hint. Updated the code according to this. | |
| app/renamedialog.h | ||
| 5 | Ok, now I got it. ;) | |
| app/fileoperations.cpp | ||
|---|---|---|
| 237 | Here dialog can be a nullptr. | |
Comment Actions
Check that dialog still exists
| app/fileoperations.cpp | ||
|---|---|---|
| 237 | You're right, this should be checked. | |
Comment Actions
Wow, more than six years after your comment you actually submitted a Diff!
Seems it was fixed at that time by introducing InputDialog in gwenview v1.
I don't know what happened on the way to v2.
As far as I understand, Gwenview was almost completely rewritten, i.e. the KDE3 version (where it works) is totally different code and UI wise from the KDE4 (+later) version.
That's a legitimate question to ask in order not to play "feature ping-pong". However, in this case I suspect it was just an oversight or simply lack of time that the feature (not the actual file) was lost.
Does it seem worth it to re-introduce this functionality without adding more source files?
Subclassing and having separate files for this class is the standard approach when using .ui files, I think that's just fine. (Personally, I feel those make the code harder to read and review, see below for why…. Still, it's done a lot in Gwenview, so not wrong per se.)
| app/fileoperations.cpp | ||
|---|---|---|
| 234 | const | |
| 236 | Perhaps dialogAccepted? Also: const | |
| 241 | const | |
| app/renamedialog.cpp | ||
| 48 | Please use the new-style connect syntax: connect(d->mFilename, &QLineEdit::textChanged, this, &RenameDialog::updateButtons); With this, you could probably even get rid of Q_SLOTS for updateButtons. | |
| 61 | const | |
| 63 | Would it be worth adding an else with selectAll()? This way it would behave like in Browse mode and like in Dolphin. | |
| 77 | Maybe call this enableButton to make it easier to read? Also: const | |
| app/renamedialog.h | ||
| 41 | Add explicit? | |
| 45 | Add const at the end. | |
| app/renamedialog.ui | ||
| 20 | This should not be needed, in particular the height restriction might be wrong with other widget styles or font sizes. | |
| 39 | As you set this in the code already, could this be marked as not-needed-for-translation? | |
| 52 | According to the HIG, the button should be called Rename. (I know the old dialog also had Ok, but let's do it right this time. The KDE3 version also had Rename, BTW.) | |
Comment Actions
added a couple of const
changed connect to new syntax
dialog now selects whole filename, if there is no extension
changed ok-button label
Comment Actions
I really should take better care of the const... ;)
| app/renamedialog.cpp | ||
|---|---|---|
| 51 | Dolphin uses a check mark icon here (dialog-ok-apply). The gwenview dialogs (resize, crop, copy, etc.) use their menu icons. | |
| app/renamedialog.ui | ||
| 20 | Size policy seems to be ignored and does not work to fix vertical size. | |
| app/renamedialog.cpp | ||
|---|---|---|
| 51 | Given that the button title is "Rename", I think the rename icon is perfectly appropriate. Feel free to submit patch to Dolphin changing it there so we can make this consistent! | |
Comment Actions
Accepting for now, but before committing, let's wait how D9731 plays out. Due to the i18n changes your patch is headed for master / 18.04 anyway.
| app/renamedialog.cpp | ||
|---|---|---|
| 51 | Yup, having the rename icon and being consistent with the other operations is fine, I guess. After all, in KDE3 the button had the rename icon, too. | |
| app/renamedialog.ui | ||
| 20 | Perhaps you have to explicitly set a size (e.g. the sizeHint) before the Fixed policy has any effect? Anyway, I played around a bit with different icon sizes, font sizes and widget styles. Seems like Qt respects the minimum sizes of the contained widgets before looking at the minimum size of the container. This means your original approach was just fine and you should not believe everything I say… If you want, you can change it back ;) | |
Comment Actions
If everthing is complete now, can I commit the patch?
Is there a special 18.04 branch or is this just the actual master? I can't see any remote branch named like this.
Comment Actions
Thanks for your patience, looks perfect now.
Is there a special 18.04 branch or is this just the actual master? I can't see any remote branch named like this.
You may have taken my comment too literally ;), sorry if it was confusing. Currently what will be committed to master will end up in the 18.04 release. However, at some point in time the release team will create a separate branch called Applications/18.04, after which commits to master will be released in 18.08 earliest. See gitk --all to match branches with the release schedule for past dates.
TL;DR: master