client: Mark entry as read when opening external link

PhrozenByte commented 7 months ago

Ten years after #451 (I'm feeling old...) it's time to try again to finally bring this tech-wise small but UX-wise major feature to upstream. The only reason to open the external link of an entry is to read said entry (especially when opened in the foreground - what most browsers do by default) - consequently the entry should be marked as read by default, too.

I believe that this should be default behaviour. There's still the possibility to make this an opt-in feature using a new config.ini option, but I'd strongly discourage it. If you want me to make it opt-in, let me know.

If you want to merge both #1470 and #1471, ignore this PR and merge #1472 instead. Merging any of the three PRs will automatically close the other two PRs.

Closes #1471 Closes #1472

netlify[bot] commented 7 months ago

Deploy Preview for selfoss canceled.

Name	Link
Latest commit	0a50a4ae9372aa6590293a74cd3476e77edd096c
Latest deploy log	https://app.netlify.com/sites/selfoss/deploys/656081e32928e60008896282

jtojnar commented 7 months ago

Thanks. I think this feature should not cause any issues; changes look okay on a first look; will give it more thorough review on the weekend.

Does this apply to v shortcut as well? I cannot remember if the synthetic events trigger the onclick handler

https://github.com/fossar/selfoss/blob/e0c9805050e83351059e19d56a0cd36680c35e58/client/js/templates/EntriesPage.jsx#L159 https://github.com/fossar/selfoss/blob/e0c9805050e83351059e19d56a0cd36680c35e58/client/js/templates/EntriesPage.jsx#L1055 https://github.com/fossar/selfoss/blob/e0c9805050e83351059e19d56a0cd36680c35e58/client/js/shortcuts.js#L104

jtojnar commented 7 months ago

How does this interact with right click?

PhrozenByte commented 7 months ago

Does this apply to v shortcut as well? I cannot remember if the synthetic events trigger the onclick handler

It didn't - until now. The reason it didn't work before wasn't that the event won't fire, but because the v shortcut emulated a click on the date string, not the external link. I've changed that (but didn't merge it into #1472 yet, thus converted #1472 into a draft, wanna discuss the next issue first). It works now.

This brought another issue to my attention: The external link button isn't the only element that opens {item.link}. Currently the favicon, thumbnail and date string all reference the external link, too. One could argue that it should be consistent, but one could also argue that there's an advantage in having at least one alternative that doesn't have this side effect. I tend to make the favicon (merely because of #1471 and it being so close to the title then) and thumbnail to also mark the entry as read, but not the date string; but that surely is no strong opinion, rather a feeling. What do you think? I'll change it accordingly then.

How does this interact with right click?

Right clicks aren't affected (right click doesn't fire the click event, but contextmenu). There's no way for us to hook into an "Open link" click in the context menu.

PhrozenByte commented 7 months ago

@jtojnar Can you please take a look at the following open question? Just need a quick decision, I'll implement it accordingly then.

This brought another issue to my attention: The external link button isn't the only element that opens {item.link}. Currently the favicon, thumbnail and date string all reference the external link, too. One could argue that it should be consistent, but one could also argue that there's an advantage in having at least one alternative that doesn't have this side effect. I tend to make the favicon (merely because of #1471 and it being so close to the title then) and thumbnail to also mark the entry as read, but not the date string; but that surely is no strong opinion, rather a feeling. What do you think? I'll change it accordingly then.

fossar / selfoss