Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: Mark entry as read when opening external link #1470

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PhrozenByte
Copy link
Contributor

@PhrozenByte PhrozenByte commented Nov 23, 2023

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

Copy link

netlify bot commented Nov 23, 2023

Deploy Preview for selfoss canceled.

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

@jtojnar
Copy link
Member

jtojnar commented Nov 24, 2023

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

link.dispatchEvent(event);

openSelectedArticle(selected);

selfoss.entriesPage?.openSelectedTarget();

@jtojnar
Copy link
Member

jtojnar commented Nov 24, 2023

How does this interact with right click?

@PhrozenByte
Copy link
Contributor Author

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.

ctrlKey: true,
}
);
const event = new MouseEvent('click', { ctrlKey: true, bubbles: true});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why bubble?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event handler for marking the entry as read doesn't fire otherwise. I'm not 100% sure why, because the event is dispatched on the element we registered the handler to, but without bubbling only the link opens but the handler doesn't run. Anyway, .click() and actual clicks bubble by default too, so we just make it behave the same.

@PhrozenByte
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants