kiwix / kiwix-desktop

Kiwix for Windows and GNU/Linux desktops
https://download.kiwix.org/release/kiwix-desktop/
GNU General Public License v3.0
776 stars 101 forks source link

Introduce Scroll Buttons to Replace Ones from Qt's Built-in TabBar. #1198

Closed ShaopengLin closed 2 months ago

ShaopengLin commented 2 months ago

Fix #157 This PR solves the scroll button issue using the second approach in this comment. The comment has been stale for a long time and I am using my best judgment to select the most intuitive approach.

Changes:

kelson42 commented 2 months ago

@ShaopengLin The computation deciding when the arrow should be displayed or not seems buggy. See here: image

All the tabs are visibles, so no arrow should be visible.

There is also some kind of strange blue artefact on the left arrow: image

ShaopengLin commented 2 months ago

@kelson42 I have fixed this issue with the need to introduce a refactor to the new tab 'tab'. After I fixed the issue you mentioned here, I could no longer scroll to the new tab 'tab' due to internal code and Qt limitations. With some digging, the only solution that I have is to introduce a persistent new tab button when the new tab 'tab' is hidden from view. This no longer required us to scroll to the new tab 'tab'. Could you re-test the UI?

The blue tear is a Qt built-in feature. I have disabled it.

kelson42 commented 2 months ago

By keeping adding new tabs via mouse double-click on links, I achieve a situation where I have a lots of tabs overflowed, but the right arrow is still not displayed: image

ShaopengLin commented 2 months ago

@kelson42 Should be fixed. Though I know where in the code that could have caused this, I don't seem to have the ability to double click on a link to open it in a new tab. The double click simply opens the link in the current tab for me. Is there a trick to doing this?

kelson42 commented 2 months ago

@kelson42 Should be fixed. Though I know where in the code that could have caused this, I don't seem to have the ability to double click on a link to open it in a new tab. The double click simply opens the link in the current tab for me. Is there a trick to doing this?

double-click with the mouse middle button.

ShaopengLin commented 2 months ago

@veloman-yunkan Unfortunately as I have tried before QTabbar::scroll doesn't work properly. Even though the controller says it will scroll child widgets, the Tabs don't appear in the child list of the QTabbar. Most of the code that controls the behavior is hidden from us in the source, which needed access to private functions.

If we were to completely replicate this, we would inevitably need to connect to those left and right scroll buttons.

https://github.com/user-attachments/assets/5e7179ec-165f-4292-8ec6-7c564f34ccff

ShaopengLin commented 2 months ago

I also think that the buttons should not disappear when you fully scroll to the respective end.

@veloman-yunkan I believe greying it out is an option too. @kelson42 whats your take?

kelson42 commented 2 months ago

I also think that the buttons should not disappear when you fully scroll to the respective end.

@veloman-yunkan I believe greying it out is an option too. @kelson42 whats your take?

OK, but please ensure these arrow buttons are displyed only if we have too many tabs.

ShaopengLin commented 2 months ago

@veloman-yunkan IMO there aren't any intuitive solutions to really "scroll". These buttons should be good enough replacements for those who do not know keyboard shortcuts to switch tabs or do not have a mouse scroller. If we figure out how to actually "scroll", those can be easily adapted. I believe we can get this merged first if you are also stuck on the scrolling behavior.

@kelson42 could you verify the UI again?

kelson42 commented 2 months ago

One last tiny concern of the same kind as in #1198 (comment) (caused by the scrolling-vs-switch-to-adjacent-tab confusion) is that one is deprived of the next/previous tab buttons unless they have enough tabs open. However I don't consider it as a blocker. Just want to draw @kelson42's attention to the small change in behavior (caused by my feedback) since his last testing.

IMHO works properly like this. I will merge.