Library list pinning behavior

[sgourdas]

This refactors the behavior of tab changes, to make sure the reading list stays pinned until the user closes it (should be hidden in library at all times)

Fixes #1078

[kelson42]

@sgourdas Thank you for you PR, we will review it.

[kelson42]

Beside my comment, it looks good from a user perspective.

[sgourdas]

This was a bit more complicated than what I would want it, for code clarity, but it needs to be done and now the behavior should be the one you mentioned.

To distinguish between a double click and not handle it as two clicks as well, I had to put a check in place so single clicks check if a double click happened at the near time they were emitted.

[veloman-yunkan]

Testing revealed the following issues:

• During the very first double click the bookmark is opened both in the current tab and in a new tab.
• Selecting a bookmark with keyboard (Tab keys) and activating it with the "Enter" key opens the bookmark in a new tab. I guess that was intended.

Overall, as you noted, the code lacks clarity. I think that having three functions onItemClicked(), onItemSingleClicked() and onItemDoubleClicked() will make the design more comprehensible. onItemClicked() will be responsible solely for distinguishing single-clicks from double-clicks and calling the corresponding self-documenting handler.

Regarding the history of this PR - please make sure that it consists of only two commits with accurate titles and useful descriptions.

[veloman-yunkan]

I don't get why the reading list is incompatible with the library view. In a newly started kiwix-desktop app, why can't I see the list of bookmarks without having to open a content (or settings) tab?

[veloman-yunkan]

I just found another small inconsistency - though the reading list keeps being displayed beside a settings tab it cannot be opened or closed while on a settings tab.

[sgourdas]

I don't get why the reading list is incompatible with the library view. In a newly started kiwix-desktop app, why can't I see the list of bookmarks without having to open a content (or settings) tab?

On top of my comment in the issue, after implementing the single click behavior (open in same tab) in commit 445284a, this would not act correctly with the library view (or settings).

I just found another small inconsistency - though the reading list keeps being displayed beside a settings tab it cannot be opened or closed while on a settings tab.

Can confirm. For me, the reading list should be hidden when viewing the settings as well, but would like feedback on it.

[veloman-yunkan]

On top of my comment in the issue, after implementing the single click behavior (open in same tab) in commit 445284a, this would not act correctly with the library view (or settings).

I think that it will work. TabBar::openUrl() opens a new tab when called while the current tab is not a content tab. Thus on a library or settings tab a single-click will open a new tab, which is logical.

[sgourdas]

In that case, that is perfect if we decide that is what we want to do.

[sgourdas]

During the very first double click the bookmark is opened both in the current tab and in a new tab.

I cannot replicate this. For me it only opens a new tab with the double clicked bookmark.

Selecting a bookmark with keyboard (Tab keys) and activating it with the "Enter" key opens the bookmark in a new tab. I guess that was intended.

Yes this is intended and is supported by the previous behavior.

Overall, as you noted, the code lacks clarity. I think that having three functions onItemClicked(), onItemSingleClicked() and onItemDoubleClicked() will make the design more comprehensible. onItemClicked() will be responsible solely for distinguishing single-clicks from double-clicks and calling the corresponding self-documenting handler.

on_itemActivated handles the QListWidget::itemActivated event, whereas on_itemClicked handles the QListWidget::itemPressed event. Further splitting the functions I think will not really provide more clarity since the code for each function will be 1-2 lines. IMO better comments (and maybe variable/function naming) is the option here.

[veloman-yunkan]

on_itemActivated handles the QListWidget::itemActivated event, whereas on_itemClicked handles the QListWidget::itemPressed event. Further splitting the functions I think will not really provide more clarity since the code for each function will be 1-2 lines. IMO better comments (and maybe variable/function naming) is the option here.

The value of a function is not only in its size but also in its name. A good name for an identifier may eliminate the need for a comment. Whenever possible the code must explain itself. In our case "single-click" and "double-click" are easily understandable concepts in the use-model domain and it will be good if they are mirrored in the design of the code.

[sgourdas]

@veloman-yunkan ok, just a last comment to clear things up, with naming and amount of functions.

We have 2 events:

• QListWidget::itemActivated from which we get double click and enter key
• QListWidget::itemPressed from which we get single or middle click (and also return if it was a single click of a middle click action)

And we will refactor the logic so they are handled:

• onItemActivated handler with itemDoubleClicked() and itemEntered()
• onItemPressed handler with itemSingleClicked() and itemMiddleClicked()

Which, after adding identification of each type of click in them, operate as the title explains.

[veloman-yunkan]

@veloman-yunkan ok, just a last comment to clear things up, with naming and amount of functions.

We have 2 events:

• QListWidget::itemActivated from which we get double click and enter key

• QListWidget::itemPressed from which we get single or middle click (and also return if it was a single click of a middle click action)

And we will refactor the logic so they are handled:

• onItemActivated handler with itemDoubleClicked() and itemEntered()

• onItemPressed handler with itemSingleClicked() and itemMiddleClicked()

Which, after adding identification of each type of click in them, operate as the title explains.

I am not sure that separating itemMiddleClicked() from itemSingleClicked() is justified - after all a middle-click is just a kind of a single-click. But let me look at the code.

[veloman-yunkan]

The set of ReadingListBar::onItem*() handlers and their relations with each other still looks somewhat messy to me.

[sgourdas]

Squashed and rebased with main to clear up things. Let's recoup on how we will proceed from here.

[veloman-yunkan]

Almost there. A few final touches and we will merge.

[veloman-yunkan]

Thanks for bearing with me! I don't remember well the original shape of this PR, but I think that you should agree that it got significantly simpler and nicer.

[sgourdas]

No worries at all. In contrast, I really enjoyed it.

Maybe I could have saved us a bit of time if there weren't some IRL distractions, that cut my workflow, but I agree it turned out pretty good.