Always have quick bookmark set

[jasonhenriquez]

Always have quick bookmark set

Pull Request Type

• Feature Implementation

Related issue

closes #5051

Description

Co-authored-by: @PikachuEXE

• Changes "Add/Remove" quick bookmark icon to "regular/solid" bookmark icon instead of link/link-slash. See e897254 for discussion.
• Updates ft-icon-button disabled styling to show title on hover, use aria-disabled, and be able to show a toast on trigger (click/space/enter) when disabled. All of these are needed usability improvements to communicate what the disabled icon actually means, as our only disabled ft-icon-button case beforehand was the blink-and-you'll-miss-it disabled ft-refresh-widget icon.
• Makes an enabled Quick Bookmark icon disabled to prevent unselection (see original story for rationale), with position in all cases moved to the be the leftmost icon now (to ensure non-disabled icons are grouped).
• The playlist deletion icon is now also disabled when a playlist is Quick Bookmarked.
• Makes quick bookmark default to an empty user playlist or the most recently played playlist when no playlist is detected during startup or playlist add.
• From @PikachuEXE: Show Quick Bookmark icon on ft-list-playlist item.

Screenshots

Testing

• Have >=1 user playlists with no quick bookmark set (e.g., by removing it before switching to this PR, or deleting the quickBookmarkTargetPlaylistId entry in settings.db) and one playlist with 0 videos -> run FreeTube -> see playlist with 0 videos set as the new quick bookmark playlist -> (REG, optional) see it persists on Ctrl+R
• Have >=1 user playlists with no quick bookmark set (e.g., by removing it before switching to this PR, or deleting the quickBookmarkTargetPlaylistId entry in settings.db) -> run FreeTube -> see most recently played playlist is set as the new quick bookmark playlist -> (REG, optional) see it persists on Ctrl+R
• (REG) Have 2 user playlists -> switch quick bookmark target to the other one -> see preference reflected and retained on Ctrl+R
• Have 0 user playlists -> Import user playlists in Data Settings -> see one is selected as new quick bookmark target

Desktop

• OS: OpenSUSE
• OS Version: TW

[PikachuEXE]

How about auto picking next quick bookmark target on delete but still allow removing quick bookmark target
It's not clear what deleting playlist would do now, but the same happens in this PR
Again I am fine with changing the default to lean for specific behaviour pattern, but not with removing existing abilities

[jasonhenriquez]

How about auto picking next quick bookmark target on delete but still allow removing quick bookmark target [...] Again I am fine with changing the default to lean for specific behaviour pattern, but not with removing existing abilities

I don't think this is a tenable standard if we're counting the ability to not have a quick bookmark playlist as a feature we are obligated to preserve by merit of it being included in a prior release. I don't see any cost to the user experience by removing this capability. The cost of keeping this capability is having people accidentally click it at some point and wonder why they can't add videos to playlists like they could before. There's no reasonable cost calculation I can come up with that justifies keeping this, but I'm open to hearing any. But I'm always going to value the choice that improves the UX if the only case against it is an arbitrary standard.

[PikachuEXE]

Let me list out some issues from #5051 and what I think how this PR solves it (or not)

A. User clicks "Disable Quick Bookmark" icon (accidentally)

1. Too easy to click
2. not enough info about what it does

B. User deletes their Favorites playlist at some point and uses a different one, not realizing that the Quick Bookmark will disappear until a new playlist is set as that one

• Not enough info about what "delete playlist" means now (which now also includes disabling quick bookmark button)

This PR tries to solve A1 & A2 by making it impossible to disable the feature. I agree that it's effective but I don't like the side effect.
For B1 this PR only changes the action of disabling the quick bookmark target to auto setting a random target without user intent
So it's from delete playlist -> cannot quick bookmark -> report bug to delete playlist -> potentially quick bookmark videos into an unintended playlist -> report bug

There are other ways to solve these

• Show a prompt/page/whatever to allow user to quick bookmark when they try to quick bookmark but quick bookmark target unset (do nothing until user input received)
• Show more detailed messages before/after using disabling quick bookmark / deleting playlists
• Disable playlist deletion button with message until quick bookmark target changed
• Make it easier to see & set a quick bookmark target on user playlists view (maybe put quick bookmark on top)

[jasonhenriquez]

I'm fine with also requiring modal confirmation for changing the quick bookmark; I can add that in if you'd prefer. But I'm still not seeing a justification for why we should allow the quick bookmark to be null if the user has a user playlist. And as to improving the auto-selection of the new playlist or making it manual, I'm open to it, but would prefer that to be in a separate PR, as I think this is a sufficient solution on its own.

[PikachuEXE]

But I'm still not seeing a justification for why we should allow the quick bookmark to be null if the user has a user playlist.

Until the manual selection is in place I don't consider delete playlist -> potentially quick bookmark videos into an unintended playlist -> report bug as solved. It's insufficient IMO as this PR's current state is just changing the unexpected result (disable quick bookmark -> change target to a random playlist if any).

[jasonhenriquez]

this PR's current state is just changing the unexpected result (disable quick bookmark -> change target to a random playlist if any).

That's incorrect; disabling quick bookmark is no longer an option for this PR. That's something that some users have already done, and we would at the very least have to do programmatic selection for these people because we don't want to prompt them on startup. Only other case that unexpected programmatic selection is done is when removing the current Quick Bookmark playlist, which is an edge case, and a toast informs them of the new playlist that is set as the new target (+ it shows up on the title of the actual control).

I think that it's unfair to say this is equivalent to the current state of users not being aware that the control is present, and it makes it clearer that they can go to the page for that playlist to configure it when seeing any instance of the Quick Bookmark icon. I don't want to build out a whole new selection prompt just for the "delete currently quick bookmarked playlist" case due to the new mitigations of the new toast (Playlist {playlistName} is the new quick bookmark playlist) and the icon with title of the playlist showing up on each video. But I do want to see what others think on this one. @MarmadileManteater @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer

[efb4f5ff-1298-471a-8973-3d47447115dc]

IMO I think everything about this implementation is nicely done.

I also dont see a reason why it should be null

We could indeed let the user select what Playlist they want to use for Quick Bookmark when the Quick Bookmark playlist is deleted as it could come over as random and would require "another input" from the user to go to the actual playlist they want to have as the Quick Bookmark one.

[PikachuEXE]

I also dont see a reason why it should be null

They can want to stop using a playlist as quick bookmark target (for converting it into playlist(s) with its content) (permanently or temporarily)
I don't see a reason NOT to allow user to defer that choice to a later point when we can

[efb4f5ff-1298-471a-8973-3d47447115dc]

Everything LGTM.

I have a few Q's before approval though.

1. I do not quite understand why we have the bookmark button on 2 pages now. Its probably added for convenience on the main Playlist page but why would we keep it inside a Playlist or vice versa ?

2. Before this PR all the settings regarding to a playlist were stored inside a playlist but now one of them is also seen on the main playlist page. Couldnt that lead to confusion?

[jasonhenriquez]

I do not quite understand why we have the bookmark button on 2 pages now. Its probably added for convenience on the main Playlist page but why would we keep it inside a Playlist or vice versa ?

I think it makes sense to be able to set it inside of the playlist, because that's information & a control about the playlist grouped with other information about it. I think it's nice to be able to have the icon visible on the list item because it's useful information about a playlist. I don't have a strong opinion about whether it should still be a button on every playlist item, or instead just a disabled icon on the list item of the current Quick Bookmark target. @PikachuEXE If you have any justification for one or the other, please feel welcome to share.

[PikachuEXE]

My use case for having buttons in user playlists view is to quickly changing a quick bookmark target, add a new videos maybe (from search result, channel video list, single remote playlist, whatever), and change it back to usual "watch later" playlist (in my case I don't have one I actually unset it). It's for saving clicks especially when there are a lot of playlists (search, click instead of search, click inside, click again, then go back to user playlists page) plus what's the point not making it a button when it's already an indicator.

(Review comment about latest change to be given later)

[PikachuEXE]

The target indicator style seems different from what I have in #5082
Where the target would have an indicator which is different in icon AND color

disable deleting a bookmarked playlist

I see nothing in regards about what should be done and/or why is it disabled to be able to delete that playlist
As a desktop user I at least expect to see title, but that doesn't work for mobile well so what I have done in #5082 is to show a toast while still allowing clicking on it (style is something else)
29a3ebd (#5082)

[efb4f5ff-1298-471a-8973-3d47447115dc]

This wording of the toast message throws me off a little. This playlist is now used for quick bookmark.

The playlist was already being used as the quick bookmark one so including the now in there would mean that it wasnt before?

VirtualBoxVM_b3dlpNRq2G.mp4

[jasonhenriquez]

You can blame that one on my excessive label conservativeness. Updated now.

[efb4f5ff-1298-471a-8973-3d47447115dc]

I see Quick Bookmark and quick bookmark being used allot. Which one should be replaced to be consistent within the whole app?

[jasonhenriquez]

Majority seems to be in lowercase, although it's basically tied. Doesn't that make it re-request translations if we change it?

[efb4f5ff-1298-471a-8973-3d47447115dc]

Im not sure, either way we should still make it consistent imo.

[jasonhenriquez]

Turns out it was actually just one label (pre-existing). The others are in caps because they're in title case (see: #5000).

[PikachuEXE]

Everything seems fine except one line/question