kiwix-desktop
kiwix-desktop copied to clipboard
kiwix
Fix #832: Seperated Bookmark Button from Search Icon
Bug Reason: The Bookmark mode is not enabled if there is text in the search bar.
Edited Changes:
- Moved the Bookmark button to the right of the search bar.
- The Bookmark button is disabled on non-Zim tabs
- Users can find the Bookmark button in the Edit menu with Shortcut Ctrl + D, with Donation Shortcut now as Ctrl + Shift + D
- Refactored SearchBar to QToolbar and moved line edits and buttons within the toolbar for less coupling.
~Changes:~ ~- Separated the focus check and text existence check. Having text in the search doesn't justify disabling the~ bookmark function. ~- Making the button in search mode transparent, as it doesn't do anything and should trigger search mode. Having it being a focus-out event doesn't make sense.~ ~- Making the button in bookmark mode have focus since it is not supposed to trigger search mode.~
~Carried over bugfix: The title isn't displaying properly after these steps:~ ~1. searching which changes the page title~ ~2. removes text in the search bar~ ~3. Click on the article which should be displaying the previous(or a wrong) title.~
Fix #832
@kelson42 Are you aware if this PR or "Carried over bugfix" is related to any other existing issues? I don't seem to find any.
I think that we rather have a UI/UX bug that has to be fixed by separating the bookmark button from the search icon.
Why don't we (always) display the bookmark button on the right of the searchbar (like in Firefox)?
@ShaopengLin I think your PR works well, but @veloman-yunkan is pretty pertinent. Would that be possible to move the star on the other end of the bar like in Web browser?
I think its doable. Just to clarify do you want it inside the search bar?
Do you want a seperate PR for this? If we want it done in the same PR, I will reword the title of this PR since we are changing the UI.
@ShaopengLin Great. Yes, inside the searchbar (like in browser) and after discussion, we would prefer to have it in current PR.
@kelson42 One thing to note, Chrome browsers all have Ctrl + D as the bookmark as a bookmark shortcut. This shortcut is taken already by the donation shortcut. This might be something to discuss if we deem a shortcut necessary.
Found a bug that mouse clicks to the button is falling through to the line edit. Working on a fix...
@kelson42 One thing to note, Chrome browsers all have Ctrl + D as the bookmark as a bookmark shortcut. This shortcut is taken already by the donation shortcut. This might be something to discuss if we deem a shortcut necessary.
CTRL+D being a standard browser shortcut to bookmark, I suspect this has the priority and we should not change this. Considering that donation menus is not something used on a regular base (would be great ;), you can put another letter which is "free".
Since we might be adding a multi-zim search button as well later into the search bar, the search bar looks more like a sub-toolbar of the top widget. I am leaning toward refactoring it to a toolbar or a simple QWidget with a line edit and buttons.
This can solve a lot of the focus and mouse cursor issues, which otherwise will need manual eventFilters to handle. The buttons and line edits interactions remain cohesive as signal/slots between them can be jointly set in the widget. What do you think? @veloman-yunkan
Edit: IMHO it feels unnatural and hacky to have everything in a LineEdit. I tried the QToolBar refactor and it is so much easier to work with, that, I will just overwrite this commit.