Fix #832: Seperated Bookmark Button from Search Icon #1060
Conversation
|
@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. |
aaf17bd to
44d858c
Compare
44d858c to
5780179
Compare
|
@ShaopengLin We don't forget you... give us a bit of time please. |
5780179 to
a7ba14d
Compare
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. |
00242ba to
71dc996
Compare
|
@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... |
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". |
71dc996 to
5eb83fb
Compare
There was a problem hiding this comment.
@ShaopengLin I have rebased on git main HEAD! Then tested this new version.
It works IMO basically how it should. At this stage I was not able to find fundamental problems. But a few things should be improved and (partly) are visible in this screenshot:
First thing is that the highligted zone margin is not the same on the top and on the right. It should be tested as well for RTL languages (with ./kiwix-desktop -reverse)
The second thing is that under certain conditions, the pointer is not OK and still show the input cursor in place of the arrow pointer. We have the same problem with the magnify glass.
|
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. |
5eb83fb to
f9dc9f7
Compare
|
@veloman-yunkan Ready for tech review IMHO |
There was a problem hiding this comment.
I looked through the changes briefly but their amount was above my natural ability to comprehend them all at once. Whenever possible, changes must be made in small incremental steps (and I believe this is how you arrived at the final code starting from the original code). Will you please kindly decompose this PR into several smaller changes? Each change must make sense on its own so that it is accurately described by its commit message.
f9dc9f7 to
3e9f8f7
Compare
SearchBar becomes QToolBar instead of QLineEdit. The previous LineEdit is now a child of SearchBar
Since we will be seperate search icon from bookmark button, we remove the logic in SearchButton and simply put an equivalent QLabel in SearchBar.
Renamed SearchButton to BookmarkButton since it no longer has the search icon. Moved BookmarkButton to the right side of SearchBar
Created a ON-OFF action for Bookmark and added in Edit Menu Actions. Assigned shortcut Ctrl + D, which the Donation shortcut is now Ctrl + SHIFT + D. Action disabled for non-Zim tabs
BookmarkButton is now a QToolButton to better work with actions. Now clicks can modify menu text displays and menu displays can simulate clicks.
Redefined styles sheets for new widgets. Increased button svg size since QToolButton doesn't increase button size to be larger than source.
3e9f8f7 to
b49d6ec
Compare
Bug Reason:
The Bookmark mode is not enabled if there is text in the search bar.
Edited Changes:
Changes:- Separated the focus check and text existence check. Having text in the search doesn't justify disabling thebookmark 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 title2. removes text in the search bar3. Click on the article which should be displaying the previous(or a wrong) title.Fix #832