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
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
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,
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
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
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,
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
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
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
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
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
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.)
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
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:
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
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)
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.
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,
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 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 =
>
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,
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
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
23 matches
Mail list logo
