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: Double click on title to mark entry as read #1469

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

Conversation

PhrozenByte
Copy link
Contributor

Ten years after #450 (I'm feeling old...) it's time to try again to finally bring feature to upstream: Double click on an entry's title to mark it as read (and double click again to mark it as unread; disabled on smartphones due to the different UI concept).

I believe that this should be the default behaviour, but since it must add a delay to distinguish between single and double clicks, some people might find it obstructive, thus I've deliberately added it as opt-in using the new double_click_mark_as_read option.

This was the first time I worked with React. So please check whether the code I've written (including the handleMultiClick function and its usage, I'm not sure whether useCallback still makes sense there) is like it's expected for React.

Copy link

netlify bot commented Nov 23, 2023

Deploy Preview for selfoss canceled.

Name Link
🔨 Latest commit 430fa8f
🔍 Latest deploy log https://app.netlify.com/sites/selfoss/deploys/6560bc6498660500084e1332

@@ -523,6 +523,7 @@ span.offline-count.diff {
color: #999999;
padding-top: 7px;
padding-bottom: 7px;
user-select: none;
Copy link
Member

Choose a reason for hiding this comment

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

This will make it impossible to select the title to copy/search it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentional, because double clicks also mark text. However, thinking about it, why isn't event.preventDefault() preventing that? I'll look into this whether there's a better solution, but first let us decide what to do with the delay (see below).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I know, I usually do double click to select words.

@@ -444,7 +487,7 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa
{/* title */}
<h3
className="entry-title"
onClick={titleOnClick}
onClick={configuration.doubleClickMarkAsRead ? titleOnMultiClick : titleOnClick}
Copy link
Member

Choose a reason for hiding this comment

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

We would probably want to skip smartphones here to avoid the delay there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

event.preventDefault();
},
1: titleOnClick,
2: useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

useCallback would be needed if handler[2] was used in dependencies list of useEffect but in this case, the whole handler object is used so that needs to be wrapped in useMemo instead, see https://react.dev/reference/react/useMemo#memoizing-a-dependency-of-another-hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably miss a lot of React's basic rendering concept to truly understand that 🙈 So, please excuse if I don't understand it right away, but from your info and the docs, combined with my limited understanding, it should look like the following? 🤔

    const titleOnMultiClick = useMultiClickHandler(useMemo(() => {
        0: (event) => {
            event.preventDefault();
        },
        1: (event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }),
        2: (event) => {
            if (canWrite && !selfoss.isSmartphone()) {
                handleToggleReadClick({ event, unread: item.unread, id: item.id });
            }
        )
    }, [ canWrite, history, location, expanded, item.unread, item.id ]));

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks correct to me.

Copy link
Member

Choose a reason for hiding this comment

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

I do not like the delay. selfoss is already far too slow for my taste (still have some optimizations in the pipeline) so it cannot be the default.

The code is pretty nice and I can see how easier access to toggling read status without opening the entry would be convenient but I am still not sure double click is the right action for it. Usually double click action is used for opening e.g. file, contrasted with single click for selection. That also works without introducing delay because selection can be immediate.

Personally, I use auto_mark_as_read=1 which makes marking articles as read convenient. Though it will not help with unmarking. Maybe we could also consider other alternatives like point 3. from #969 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. How about dumping the delay?

It's just important to note that a double click then fires both events, i.e. a double click causes the entry to expand (or collapse) and marks it as read (or unread). It's a little less separation of concerns, but still quite convenient: You click once to expand the article, read it and close+mark it with a double click. If you decide not to read it yet (that's why I don't like auto_mark_as_read=1: I often decide not to read an article straight away after expanding it, but later), you just collapse it with a single click.

The only disadvantage is that one can't mark an entry as read without opening it. I can think about multiple solutions for that: The first is to additionally implement #969 (comment). Another solution might be to distinguish where the double click happened: Double clicking on the title also expands/collapses the entry, but a double click anywhere else on the header just marks it as read/unread. What do you think about that?

Copy link
Member

Choose a reason for hiding this comment

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

I am afraid that without delay, one probably could not use this with scroll_to_article_header=1. But maybe that is fine.

Generally, I am not a fan of different parts of the same element behaving differently without some affordance to distinguish those areas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this isn't going to work with scroll_to_article_header=1. So, shall we go with the delay-less solution? We can solve #969 (comment) another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtojnar Can you please take a look into this? Just need a quick decision, I'll implement it accordingly then.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, removing the delay sounds good. It should make the patch much shorter so there will be less of a chance of the new code path bitrotting, which is my primary concern.

### `double_click_mark_as_read`
<div class="config-option">

set this to `1` to mark an item as read when double clicking on it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
set this to `1` to mark an item as read when double clicking on it.
Set this to `1` to enable toggling item’ (un)read state by double clicking the title.

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.

None yet

2 participants