D29254: [RenameDialog] Add an arrow indicating direction from src to dest
This revision was automatically updated to reflect the committed changes. Closed by commit R241:baa38ebee32e: [RenameDialog] Add an arrow indicating direction from src to dest (authored by ahmadsamir). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29254?vs=81628=81728 REVISION DETAIL https://phabricator.kde.org/D29254 AFFECTED FILES src/widgets/renamedialog.cpp To: ahmadsamir, #frameworks, dfaure, meven, ngraham, #dolphin Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
meven accepted this revision. REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham, #dolphin Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir edited the summary of this revision. REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham, #dolphin Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir updated this revision to Diff 81628. ahmadsamir edited the summary of this revision. ahmadsamir added a reviewer: Dolphin. ahmadsamir removed subscribers: safaalfulaij, hpereiradacosta, pino, ngraham. ahmadsamir added a comment. Verbatim REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29254?vs=81627=81628 BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 AFFECTED FILES src/widgets/renamedialog.cpp To: ahmadsamir, #frameworks, dfaure, meven, ngraham, #dolphin Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns, safaalfulaij, hpereiradacosta, pino
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir updated this revision to Diff 81627. ahmadsamir added a comment. Take RTL layout into consideration when setting the arrow icon by using qApp->isRightToLeft() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29254?vs=81492=81627 BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 AFFECTED FILES src/widgets/renamedialog.cpp To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir added a comment. OK, thank you :) I'll adjust the diff accordingly. REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
safaalfulaij added inline comments. INLINE COMMENTS > ahmadsamir wrote in renamedialog.cpp:299 > @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how > to switch it to rtl). But I agree the code here should account for rtl anyway. > > @ngraham: I think we should stick to the icon naming spec[1], so that it > works with themes other than breeze/oxygen; so it has to be go-previous. > > [1] > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html The interface of Dolphin is RTL-aware, just the panels and the file view are not (for many usability reasons) Make sure to have Qt translations installed if the layout isn't switching automatically with the language change. REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: safaalfulaij, hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
hpereiradacosta added inline comments. INLINE COMMENTS > hpereiradacosta wrote in renamedialog.cpp:299 > dolphin --reverse > should run it in right-to-left mode > (or whatever the opposite of your current locale) you can use qApp->isRightToLeft() to check the rtl direction REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
pino added inline comments. INLINE COMMENTS > ahmadsamir wrote in renamedialog.cpp:299 > @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how > to switch it to rtl). But I agree the code here should account for rtl anyway. > > @ngraham: I think we should stick to the icon naming spec[1], so that it > works with themes other than breeze/oxygen; so it has to be go-previous. > > [1] > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html > (or if it is, I couldn't find out how to switch it to rtl). $ dolphin --reverse it is provided directly by QApplication, it works also in Qt-only applications REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
hpereiradacosta added inline comments. INLINE COMMENTS > ahmadsamir wrote in renamedialog.cpp:299 > @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how > to switch it to rtl). But I agree the code here should account for rtl anyway. > > @ngraham: I think we should stick to the icon naming spec[1], so that it > works with themes other than breeze/oxygen; so it has to be go-previous. > > [1] > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html dolphin --reverse should run it in right-to-left mode (or whatever the opposite of your current locale) REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir marked an inline comment as done. ahmadsamir added inline comments. INLINE COMMENTS > pino wrote in renamedialog.cpp:299 > this is not right-to-left aware; please use the layout direction of the > widget to use "go-next" or "go-previous" @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how to switch it to rtl). But I agree the code here should account for rtl anyway. @ngraham: I think we should stick to the icon naming spec[1], so that it works with themes other than breeze/oxygen; so it has to be go-previous. [1] https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ngraham added inline comments. INLINE COMMENTS > pino wrote in renamedialog.cpp:299 > this is not right-to-left aware; please use the layout direction of the > widget to use "go-next" or "go-previous" (`go-next-rtl-symbolic`, not `go-previous`; that would be semantically inaccurate.) REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
pino added inline comments.
INLINE COMMENTS
> renamedialog.cpp:299
> +// direction from src to dest
> +const QPixmap pix =
> QIcon::fromTheme(QStringLiteral("go-next")).pixmap(d->m_srcPreview->height());
> +srcToDestArrow->setPixmap(pix);
this is not right-to-left aware; please use the layout direction of the widget
to use "go-next" or "go-previous"
REPOSITORY
R241 KIO
BRANCH
l-srt-to-dest (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D29254
To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
meven accepted this revision. meven added a comment. This revision is now accepted and ready to land. LGTM REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir marked an inline comment as done. ahmadsamir added inline comments. INLINE COMMENTS > meven wrote in renamedialog.cpp:143 > You don't need a member variable for this. Indeed, it's only used in one place (too much copy/paste...). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir updated this revision to Diff 81492. ahmadsamir added a comment. Don't use a member var for an object that's only used in one function REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29254?vs=81431=81492 BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 AFFECTED FILES src/widgets/renamedialog.cpp To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
meven requested changes to this revision. meven added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > renamedialog.cpp:143 > QLabel *m_destPreview; > +QLabel *m_srcToDestArrow; > QScrollArea *m_srcArea; You don't need a member variable for this. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ngraham accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir added a comment. I meant it worked with Arabic, the direction was the same, just the text is translated, so the renamedialog and dolphin aren't RTL AFAICS. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ngraham added a comment.
Maybe this helps for RTL?
https://doc.qt.io/qt-5.9/qtwidgets-tools-i18n-mainwindow-cpp.html
INLINE COMMENTS
> renamedialog.cpp:302
> +const int size = d->m_srcPreview->height();
> +const QPixmap pix =
> QIcon::fromTheme(QStringLiteral("go-next")).pixmap(size);
> +d->m_srcToDestArrow->setPixmap(pix);
If you figure out how to make it RTL aware, you could toggle between the
`go-next-symbolic` and `go-next-rtl-symbolic` icons
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D29254
To: ahmadsamir, #frameworks, dfaure, meven
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir added a comment. I tried with Arabic, and the rename dialog had some Arabic text, but the layout was still LTR (can it switch to RTL?). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29254 To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29254: [RenameDialog] Add an arrow indicating direction from src to dest
ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, meven. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY This adds a visual indication to show the direction of the copy/move..etc operation pointing from source to destination. See the screenshot. BUG: 268600 REPOSITORY R241 KIO BRANCH l-srt-to-dest (branched from master) REVISION DETAIL https://phabricator.kde.org/D29254 AFFECTED FILES src/widgets/renamedialog.cpp To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
