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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 55 additions & 12 deletions client/js/templates/Item.jsx
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

@PhrozenByte PhrozenByte Nov 26, 2023

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.

Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,30 @@ function setupLightbox({
}));
}

function useMultiClickHandler(handler, delay = 400) {
const [state, setState] = useState({ clicks: 0, args: [] });

useEffect(() => {
const timer = setTimeout(() => {
setState({ clicks: 0, args: [] });

if (state.clicks > 0 && typeof handler[state.clicks] === 'function') {
handler[state.clicks](...state.args);
}
}, delay);

return () => clearTimeout(timer);
}, [handler, delay, state.clicks, state.args]);

return (...args) => {
setState((prevState) => ({ clicks: prevState.clicks + 1, args }));

if (typeof handler[0] === 'function') {
handler[0](...args);
}
};
}

function stopPropagation(event) {
event.stopPropagation();
}
Expand Down Expand Up @@ -104,7 +128,7 @@ function closeFullScreen({ event, history, location, entryId }) {
}

// show/hide entry
function handleClick({ event, history, location, expanded, id, target }) {
function handleToggleOpenClick({ event, history, location, expanded, id, target }) {
const expected = selfoss.isMobile() ? '.entry' : '.entry-title';
if (target !== expected) {
return;
Expand All @@ -123,6 +147,14 @@ function handleClick({ event, history, location, expanded, id, target }) {
}
}

// mark entry read/unread
function handleToggleReadClick({ event, unread, id }) {
event.preventDefault();
event.stopPropagation();

selfoss.entriesPage.markEntryRead(id, unread == 1);
}

// load images
function loadImages({ event, setImagesLoaded, contentBlock }) {
event.preventDefault();
Expand Down Expand Up @@ -257,6 +289,8 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa
[currentTime, item.datetime]
);

const canWrite = useAllowedToWrite();

const previouslyExpanded = usePreviousImmediate(expanded);
const configuration = useContext(ConfigurationContext);

Expand Down Expand Up @@ -360,15 +394,30 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa
}, [configuration, expanded, item.id, item.unread, previouslyExpanded]);

const entryOnClick = useCallback(
(event) => handleClick({ event, history, location, expanded, id: item.id, target: '.entry' }),
(event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry' }),
[history, location, expanded, item.id]
);

const titleOnClick = useCallback(
(event) => handleClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }),
(event) => handleToggleOpenClick({ event, history, location, expanded, id: item.id, target: '.entry-title' }),
[history, location, expanded, item.id]
);

const titleOnMultiClick = useMultiClickHandler({
0: (event) => {
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.

(event) => {
if (canWrite && !selfoss.isSmartphone()) {
handleToggleReadClick({ event, unread: item.unread, id: item.id });
}
},
[canWrite, item.unread, item.id]
)
});

const starOnClick = useCallback(
(event) => {
event.preventDefault();
Expand All @@ -379,12 +428,8 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa
);

const markReadOnClick = useCallback(
(event) => {
event.preventDefault();
event.stopPropagation();
selfoss.entriesPage.markEntryRead(item.id, item.unread == 1);
},
[item]
(event) => handleToggleReadClick({ event, unread: item.unread, id: item.id }),
[item.unread, item.id]
);

const loadImagesOnClick = useCallback(
Expand All @@ -411,8 +456,6 @@ export default function Item({ currentTime, item, selected, expanded, setNavExpa
[item.source]
);

const canWrite = useAllowedToWrite();

const _ = useContext(LocalizationContext);

const sharers = useSharers({ configuration, _ });
Expand Down Expand Up @@ -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.

>
<span
className="entry-title-link"
Expand Down
1 change: 1 addition & 0 deletions client/styles/main.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

.entry-title a {
Expand Down
6 changes: 6 additions & 0 deletions docs/content/docs/administration/options.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,12 @@ Hide read articles on mobile devices.
Set to `0` to stop the interface from scrolling to the article header when an article is opened.
</div>

### `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.

</div>

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

Expand Down
1 change: 1 addition & 0 deletions src/controllers/About.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public function about(): void {
'autoHideReadOnMobile' => $this->configuration->autoHideReadOnMobile, // bool
'scrollToArticleHeader' => $this->configuration->scrollToArticleHeader, // bool
'showThumbnails' => $this->configuration->showThumbnails, // bool
'doubleClickMarkAsRead' => $this->configuration->doubleClickMarkAsRead, // bool
'htmlTitle' => trim($this->configuration->htmlTitle), // string
'allowPublicUpdate' => $this->configuration->allowPublicUpdateAccess, // bool
'publicMode' => $this->configuration->public, // bool
Expand Down
2 changes: 2 additions & 0 deletions src/helpers/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ class Configuration {

public bool $showThumbnails = true;

public bool $doubleClickMarkAsRead = false;

public int $readingSpeedWpm = 0;

/**
Expand Down