Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buttons in toolbar drawn incorrectly in right-to-left layout #594

Open
asashnov opened this issue Feb 12, 2021 · 10 comments · May be fixed by #797
Open

Buttons in toolbar drawn incorrectly in right-to-left layout #594

asashnov opened this issue Feb 12, 2021 · 10 comments · May be fixed by #797
Assignees
Labels
bug UI User Interface
Milestone

Comments

@asashnov
Copy link
Contributor

asashnov commented Feb 12, 2021

Found on current 'master' (ca776b4)

Steps to reproduce:

  1. Run Kiwix application with right-to-left interface layout by adding -reverse CLI option
  2. See the buttons in toolbar
  3. Open any .zim file from library
  4. See the buttons in toolbar again

Actual result:

Screenshot from 2021-02-12 20-10-29

Screenshot from 2021-02-12 20-10-38

Expected result:

Graphical items in toolbar should not overwrite each other, no duplicate drawings.

Additional info

There is another issue about Back/Forward buttons order: #595

@kelson42
Copy link
Collaborator

@asashnov and the icons should be on the right!!!

@asashnov
Copy link
Contributor Author

asashnov commented Feb 12, 2021

@kelson42 Which icons did you ment in your previous comment?

Icons inside tab headers (close button and the content web page icon) are reverted- that's right, this is a standard behaviour of QTabbar. At least, on my Linux.

On Windows it can be different, need to check with Kiwix.

One of the example I manage to find on the Internet with Arabic interface is ESET:

web_phishing_zoom75

Source: https://help.eset.com/ees/6/ar-EG/index.html?idh_config_antiphish.htm

@asashnov
Copy link
Contributor Author

asashnov commented Feb 12, 2021

Qt manual says (https://doc.qt.io/qt-5/qtquick-positioning-righttoleft.html):

The general rule of thumb is that content (like photos, videos and maps) is not mirrored, but positioning of the content (like application layouts and the flow of visual elements) is mirrored.
...
Also, there are some special cases you may need to take into account where right-to-left language speakers are used to left-to-right positioning, for example when using number dialers in phones and media play, pause, rewind and forward buttons in music players.

But it says nothing clearly about tabs header.

So I'm confused here. I think we can rely on the default behaviour of QTabbar now, and may setLayoutDirection(Qt::LeftToRight); in Tabbar constructor if users ask us about it:

Screenshot from 2021-02-13 00-47-23

@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jun 2, 2021
@kelson42 kelson42 added this to the 2.2.0 milestone Nov 13, 2021
@stale stale bot removed the stale label Nov 13, 2021
@kelson42
Copy link
Collaborator

kelson42 commented Dec 4, 2021

@asashnov One #525 is fixed, we shoukd really fix this one.

@kelson42 kelson42 modified the milestones: 2.2.0, 2.3.0 Feb 14, 2022
asashnov added a commit that referenced this issue Feb 20, 2022
There was a hacky implementation which created QPushButton as a child of
QLineEdit, but without using any Layout (actually, QLineEdit has no
documented facilities to contain any other widget).
So, somehow in old implementation Qt placed QPushButton in the left side
of QLineEdit. QLineEdit itself didn't knew about it, so in CSS
left padding 40px; was needed. With '-reverse' CLI option (right-to-left
layout) padding 40px remained to be on the left. Also, depending on which
text we enter in QLineEdit (English or Arabic) the text can also be started
from the left or the right side of text area in QLineEdit, so otherwise
we had to detect text layout and update CSS paddings on the fly.

The new implementation is more elegant and uses the documented facility of
QLineEdit to contain an QAction. So no hacks with CSS padding is need.
Also, because QPushButton was a separate QWidget, clicking on it didn't
pass the focus into QLineEdit, but clicking on QAction icon does focusIn.
So focus interation code also has to be changed.

Fixes: #594
@asashnov asashnov linked a pull request Feb 20, 2022 that will close this issue
@kelson42 kelson42 modified the milestones: 2.3.0, 2.2.0 Feb 21, 2022
@kelson42
Copy link
Collaborator

@asashnov Sorry for my lack of feedback. I did not meant the tab headers. I meant the context of the search box:

  • The text should be aligned right (like the suggestions)
  • Any magnify logo should be on the right as well

@kelson42 kelson42 modified the milestones: 2.2.0, 2.3.0 Mar 3, 2022
asashnov added a commit that referenced this issue Mar 4, 2022
There was a hacky implementation which created QPushButton as a child of
QLineEdit, but without using any Layout (actually, QLineEdit has no
documented facilities to contain any other widget).
So, somehow in old implementation Qt placed QPushButton in the left side
of QLineEdit. QLineEdit itself didn't knew about it, so in CSS
left padding 40px; was needed. With '-reverse' CLI option (right-to-left
layout) padding 40px remained to be on the left. Also, depending on which
text we enter in QLineEdit (English or Arabic) the text can also be started
from the left or the right side of text area in QLineEdit, so otherwise
we had to detect text layout and update CSS paddings on the fly.

The new implementation is more elegant and uses the documented facility of
QLineEdit to contain an QAction. So no hacks with CSS padding is need.
Also, because QPushButton was a separate QWidget, clicking on it didn't
pass the focus into QLineEdit, but clicking on QAction icon does focusIn.
So focus interation code also has to be changed.

Fixes: #594
asashnov added a commit that referenced this issue Apr 19, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
asashnov added a commit that referenced this issue Apr 19, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
@stale
Copy link

stale bot commented May 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label May 2, 2022
asashnov added a commit that referenced this issue May 16, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
asashnov added a commit that referenced this issue Jun 14, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
asashnov added a commit that referenced this issue Jun 15, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
kelson42 pushed a commit that referenced this issue Jun 24, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
kelson42 pushed a commit that referenced this issue Jul 2, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
@kelson42 kelson42 modified the milestones: 2.3.0, 2.2.3 Jul 2, 2022
@stale stale bot removed the stale label Jul 2, 2022
kelson42 pushed a commit that referenced this issue Jul 24, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
@kelson42 kelson42 modified the milestones: 2.3.0, 2.4.0 Sep 8, 2022
kelson42 pushed a commit that referenced this issue Sep 10, 2022
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
kelson42 pushed a commit that referenced this issue May 11, 2023
Use QLineEdit::addAction() to place a button inside the widget.
It will automatically positioned on the left, or on the right, depending
on the current language.

Fixes: #594
@kelson42 kelson42 modified the milestones: 2.4.0, 2.5.0 Sep 30, 2023
@ShaopengLin
Copy link
Collaborator

@kelson42 May I have a summary on what is left to do? They seem to be looking ok in reverse mode on Linux.

I did spot this one, where the text is wrongly shadowed as if it is still in left-to-right mode:
Screenshot from 2024-06-20 15-51-59
Screenshot from 2024-06-20 16-01-52

@kelson42
Copy link
Collaborator

@ShaopengLin Please rebase and then I will do a new review

@ShaopengLin
Copy link
Collaborator

@ShaopengLin Please rebase and then I will do a new review

You might have commented on the wrong place. I assume you meant to put it on the save page PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug UI User Interface
Projects
Status: DOING
Development

Successfully merging a pull request may close this issue.

3 participants