edrlab / thorium-reader

A cross platform desktop reading app, based on the Readium Desktop toolkit
https://www.edrlab.org/software/thorium-reader/
BSD 3-Clause "New" or "Revised" License
1.68k stars 148 forks source link

misleading "bookmark suppression" icon in some cases #2340

Closed llemeurfr closed 1 month ago

llemeurfr commented 2 months ago

If I'm on a screen where there are already several bookmarks and I press the bookmark button without having selected any text, nothing happens, even though overing on the bookmark button displayed an icon indicating that a bookmark would be deleted. If I select text beforehand, it adds a bookmark (whereas the bookmark button indicated a delete action). So the icon displayed on mouse hover in this case (bookmarks already existing in the screen) is misleading.

Compare this to v2.4: the behaviour was the same, but there was no change to the icon on mouseover; there was no information, not a misleading information.

Note: the "bookmark suppression" icon is useful if there is only one bookmark on the screen : in this case this bookmark will be deleted by hitting the button.

llemeurfr commented 1 month ago

I see a difference with the latest beta:

Is it possible to show a "delete" bookmark icon in the first case, a "plus" bookmark button (to be created) in the second and let the bookmark button unchanged (active) in the third case?

panaC commented 1 month ago

https://github.com/edrlab/thorium-reader/blob/ea0f1c373dc88bdeb07d8c6de246aa64af8dbb20/src/renderer/reader/components/ReaderHeader.tsx#L907

https://github.com/edrlab/thorium-reader/blob/ea0f1c373dc88bdeb07d8c6de246aa64af8dbb20/src/renderer/reader/components/Reader.tsx#L774

https://github.com/edrlab/thorium-reader/blob/ea0f1c373dc88bdeb07d8c6de246aa64af8dbb20/src/renderer/reader/components/Reader.tsx#L2465

https://github.com/edrlab/thorium-reader/blob/ea0f1c373dc88bdeb07d8c6de246aa64af8dbb20/src/renderer/reader/components/Reader.tsx#L2287

we need to pass the boolean array length of visibleBookmarkList from the Reader.tsx local react component state to the ReaderHeader.tsx component, or move the visibleBookmarkList logic close to the bookmark input component.