Page bookmarks #5003
Page bookmarks #5003
Conversation
github-actions
bot
added
the
PR: waiting for review
kommunarr
force-pushed
the
feat/add-page-bookmarking
branch
from
8efa37b to
21601ed
Compare
Reduces chance that the star is seen as a button or control.
kommunarr
force-pushed
the
feat/add-page-bookmarking
branch
from
21601ed to
7896b7b
Compare
github-actions
bot
added
PR: merge conflicts / rebase needed
and removed
PR: waiting for review
This comment was marked as outdated.
This comment was marked as outdated.
…at/add-page-bookmarking
This comment was marked as duplicate.
This comment was marked as duplicate.
|
This feature differs quite a bit from what I imagine it to be. Lets take the following artistic masterpiece as a starting point (this applies to #1505 and #1492):
ONLY when the user clicks on an empty search bar (imagine the search bar being empty in the pic above):
When the user starts to type into the search bar:
Debatable:
I dont see the need for:
|
Addressing #1492 is out of scope for this PR, IMO, unless we want to make it even bigger.
I disagree on this one. This sets a hard limit on the number of bookmarks, makes searching bookmarks harder, and is different from most browser (and in the case of YT for search history) behavior that people are most familiar with, where those are put at the top of the results.
I disagree with this one for the reason that it's a prohibitive UX. You don't often think to save a page before you actually click on it, which is going to lead to a good deal of frustration. You probably want it to be somewhere visually constant so that you can bookmark a page whenever you want to. See my discussion from the OP:
See answers to the above on why I disagree. In short, the cognitive model of browser bookmarking is one that users know about and are more effortlessly familiar with, but the model you're proposing is one that users will have to put in some work to learning and understanding the behaviors you've laid out, as it's a novel (at least to my knowledge) system and ruleset.
I can see the argument for this one. I would be open to having the modal close action for this one be to save rather than cancel (similar to Firefox), but I do worry that's not as intuitive given that differs from our other modals. Alternatively, we could have bookmarking not open the modal by default, but a toast pops up presenting you the option to edit it (otherwise, you can just click the icon again). |
This comment was marked as outdated.
This comment was marked as outdated.
Included that one just to make the examples easier to understand
Ah i didnt know. How does YT search history work? Could you maybe expand some more on the how browsers tackles bookmarks and how we do a similar job of it (i havent used browser bookmarks for a long time)
Ah ok I think I understand
👍 im fine with it but please provided some examples of the familiar behaviors like requested above.
After testing some more i withdraw this. I think its very valuable to search for something with a filter applied and bookmark it with an alias so you exactly know what it is |
Incidentally, I'm thinking about us moving the star icon to the left of the label, adding a magnifying glass by regular results, adding a loop icon for history results, and adding remove icon or text button on the right side for history results.
Some behaviors vary based off of the specific browser and what settings you have enabled, but at least in every major browser, bookmarks and search history are prioritized over regular results by default. See discussion here on Chromium's behavior for example. As to which takes precedence over which, I think it varies. In Firefox, judging from a few minutes of testing, bookmarks and search history are equal, with relevance, closeness of order, and time of search seeming to be the apparent factors of sorting in order of priority descending.
This is grabbed from Firefox:
|
…at/add-page-bookmarking
| export function getIconForRoute(route) { | ||
| const routeSlashIndex = route.indexOf('/', 2) | ||
| const truncatedRoute = (routeSlashIndex === -1) ? route : route.substring(0, routeSlashIndex) | ||
| switch (truncatedRoute) { | ||
| case '/subscriptions': | ||
| return ['fas', 'rss'] | ||
| case '/subscribedchannels': | ||
| case '/channel': | ||
| return ['fas', 'list'] | ||
| case '/trending': | ||
| return ['fas', 'fire'] | ||
| case '/popular': | ||
| return ['fas', 'users'] | ||
| case '/userplaylists': | ||
| return ['fas', 'bookmark'] | ||
| case '/history': | ||
| return ['fas', 'history'] | ||
| case '/settings': | ||
| return ['fas', 'sliders-h'] | ||
| case '/about': | ||
| return ['fas', 'info-circle'] | ||
| case '/search': | ||
| return ['fas', 'magnifying-glass'] | ||
| case '/hashtag': | ||
| return ['fas', 'hashtag'] | ||
| case '/post': | ||
| return ['fas', 'message'] | ||
| case '/playlist': { | ||
| const solidOrRegular = route.includes('?playlistType=user') ? 'fas' : 'far' | ||
| return [solidOrRegular, 'bookmark'] | ||
| } case '/watch': | ||
| return ['fas', 'play'] | ||
| default: | ||
| return null | ||
| } |
There was a problem hiding this comment.
I haven't tested this pull request yet but this list includes a lot of routes that there is no point bookmarking, as they have icons in the side bar.
I haven't looked into the latest changes on this pull request yet, but last time I looked at it I was planning to veto it entirely, as it was so far away from what it should have been, just saving search results like people asked.
There was a problem hiding this comment.
I have just seen your justifications above about you wanting to follow web browser behaviour. FreeTube is a desktop app, not a web browser, it makes sense to be able to bookmark any page in a web browser as you can open anything on the internet, there is no point in doing that in FreeTube and should frankly be considered broken behaviour.
There was a problem hiding this comment.
The reason I kept icons for all routes is because I intend to use these for navigation history. See comment here.
There was a problem hiding this comment.
[I]t makes sense to be able to bookmark any page in a web browser [...] there is no point in doing that in FreeTube
This is not the current behavior in this PR, and this indicates to me that you have not actually tested it in its current state before reaching your conclusions. From what I can gather, from a purely functional perspective, your opinion is that the array below should just include Search Results and Hashtag, at which point some other tweaks to the implementation & UX could be made as well if we're 100% certain are the only two we'll ever be concerned about.
FreeTube/src/renderer/components/top-nav/top-nav.js
Lines 50 to 58 in 5ddfbcc
My disagreement, that is seemingly shared by at least a few other team members, is that it doesn't present a detriment to the user experience to also allow users to save other relevant content like posts, channels, videos, playlists, and linkable settings sections / settings searches (TBD) for easy use if they so prefer. I'll provide counterarguments to three concerns to this approach now.
- Some people will not use this feature. This is, in absence of additional harms applied to those users in exchange (a separate concern I will address later on), a neutral and acceptable state of affairs.
- Adding a new control will be confusing or otherwise materially harm the user experience by merit of it being present. This is a valid concern insofar as the existence of any new control is something that can risk contributing to clutter or comprehension problems. This is where I believe there was a misunderstanding with regards to my rebuttal I provided to this a while back:
I have just seen your justifications above about you wanting to follow web browser behaviour [...] there is no point in doing that in FreeTube and should frankly be considered broken behaviour.
The claim I was making was not "if web browsers do it, let's copy what they do." Rather, I was addressing the comprehension concern that users would have comprehension issues learning what this feature is and how to use it, such that it would negatively contribute to their user experience. To clarify, this means that this is very easy to learn how to use and interact with as a control compared to any alternative access method that has been suggested so far.
To bring it back to:
It's so far away from what it should have been, just saving search results
What does the mental model look like for "save an arbitrary search so I can go back to it later?" and the corresponding subproblems? The supposedly more modest alternative is actually more foreign and harder to learn & use to most users! Whereas this is something that anyone knows how to use because we're appropriating the same mental model.
- (Philosophical claim) There is not a strict need for this, either because it has low salience or that there are / will be alternative access methods for these five additional types of content such that presenting this new access method is problematic. Please correct me if I'm wrong, but I believe this is the main thing that you're hinting at with this quote:
[T]here is no point in doing that in FreeTube and should frankly be considered broken behaviour.
This is a philosophical disagreement that I don't think you will readily change your mind on, but I do think I can at least try to address some relevant factors here.
i. I do believe there is a bit of a personal bias in that you do not intend to use this feature yourself. This in itself fine, as I am also biased as someone who wants to use this feature, so I don't think holding that bias is inherently a bad thing.
ii. Related to i, you think that the user base at large would feel the same. This one neither of us can discuss definitively because you and I both have dead zones for how we use the app & our priorities versus the user base at large. From a democratic perspective, let's say that we cancel each other out so far, thus leaving the rest of the team to be the deciders of ii.
iii. Related to i and ii but not exactly, the idea seems to be that if we have other access methods for these forms of content (e.g., subscribing to a channel, saving a video to a playlist), adding a new one for them is buggy, wasteful, and/or a faux pas. This is something I addressed earlier, but what I disagree with in this philosophical claim is that it originates from a place of "if the users just interacted with the app the way we told them to, they wouldn't need this additional stuff." Even if everything is possible through a rigid access method, that doesn't make it the only way that users can or should go about doing it. I'll link to my earlier reply where I addressed this in more depth, but needless to say, I do think there is room for strong & legitimate disagreement with this philosophical claim.
As it stands, from the inventory above, my assessment is that most of your disagreement is represented by philosophical disagreement in point number 3 and its subclaims i-iii. Please correct me if I'm wrong on that, as I'm sure there might be more to your perspective, but you have not elucidated it yet, so I am going off of the available information you have provided. If I am correct that it is mainly philosophical concerns, I do think from a democratic perspective we should leave it to the rest of the team to determine whether to merge this or not, with you and me only providing advisory opinions in terms of the philosophical disagreements.
absidue
added
U: Waiting for Response from Author
PR: changes requested
and removed
PR: waiting for review
|
After catching up on the thread, it looks like some of the problems that made me want to veto this pull request in the first place still exist (thankfully you conceded that this is not the right solution for saving playlists, so that's one thing at least) and that you are unwilling to budge on those remaining things and I am unwilling to budge from my position of not wanting to merge a pull request with major flaws. So it seems like we are at an impasse. |
|
I am doing my best to be a team player and respond constructively to specific & actionable concerns wherever possible. Let's both do what we can to navigate that, as I am currently not being given a lot to work with here other than "👎". I will say if it's mostly just philosophical disagreements, I think you should at worst leave it as non-blocking feedback and allow the rest of the team to merge it if they are in unanimity. |
There was a problem hiding this comment.
Incomplete code review.
I'm still opposed to adding multiple ways to do the same things, we already have more powerful ways of
saving channels and videos, via subscribing and adding to a playlist respectively. I also see no point in saving a link to the settings, we already have the settings themselves linked in the sidebar and when you visit the setting's page we have the side bar that you implemented that lets you jump to specific sections. If you want to search for settings lets implement a proper settings search on the settings page, not this.
src/renderer/components/page-bookmark-prompt/page-bookmark-prompt.js
Outdated
Show resolved
Hide resolved
src/renderer/components/page-bookmark-prompt/page-bookmark-prompt.js
Outdated
Show resolved
Hide resolved
I don't know if we'll ever reach full agreement on this philosophical claim, and I think that's okay in itself. If I could provide a bit more reasoning on why I disagree with this for practical reasons, here's a bit of it: While I obviously don't have concrete metrics on this, anecdotally speaking, there is a sizable contingent of YT / Piped / etc. users who are bookmarkers, i.e., they use their browser's bookmarking feature for saving and organizing videos or relevant pages. While some of this can be attributed to beliefs that it's relatively more private, the more pertinent explanation for a lot of those users is "I just like doing it more that way." In reality, that explanation likely has layers of "YT's UX is more confusing for me," "I'm familiar with the mental model of saving things in bookmarks," "YT's UX takes more time or searching in comparison to the way that I use it." Now, one answer to these users from FreeTube could be that they're dinosaurs, and they just have to give that up if they want to use our app, as we already have access methods for any kind of page. The problem is that these folks, as much as we may not agree with them in full, actually have some merit to the friction they feel. For example, that a method exists for doing something != it's the best experience doing that something. One could say "oh, you can already save things through X way" - but it's not just about saving: it's finding, retrieving, accessing, and archiving in the common language & access pattern of pages. If there's a channel I'm in the process of binging, I could spend a half minute scrolling through my subscribed channels to find it in, or go to the Channels tab and search the name, or search their name in the search bar and try to find it in the results, but not all of these are perfect & intuitive to most users, especially if it's not something they've consciously thought about (note: rule one of UX design: don't make me think). If there's one particular video I'm thinking of that I want to watch, I could of course a) toggle the "Playlist with Matching Videos" setting, search a keyword, click on that playlist, and click on the particular video that matches that keyboard, or b) put what I remember of the title in the search bar and hope to find it in the results, but again, I think it's fair to say that's not the best or most intuitive solution for most people — especially given that they are not as familiar with the app as you and me. If there's a particular remote playlist or UULF-channel-as-playlist I like to watch, we don't currently have any good solutions, but we could theoretically decide to emulate the idea of saving remote playlists like YouTube has as a feature — I'm not fully sold on it as an idea / experience, but even if/when we do implement it, it will be more or less the same functionality — bookmarking a link to a certain kind of page that you have to go to "certain kind of page" section (Playlists) to get to. The main point I'm getting at here with these — alongside a few other kinds of saveable content that we don't have any existing means to easily save & return to — is that there are theoretically "most powerful" ways to save and organize a few types of content in FreeTube, but the mental models and access methods are anything but homogeneous, thus introducing a steeper learning curve to getting to know & use the app (harming the UX for the majority of users). Even for power users, the speed, convenience, & level of cognition required for finding, retrieving, accessing, & archiving each different kind of content are harmed by their heterogeneity. I posit that this is why many YouTube users choose — either partially or to a great extent — their browser's bookmarking functionality in lieu of YouTube's features for everything. I further assert that people who think FreeTube's UX can initially and/or occasionally be subtly frustrating to use is at least somewhat attributable to this unfilled usability gap.
I do agree more with this particular part based on considering your reasoning and will remove that for now. The initial idea was that if we have linkable settings searches, rather than the flow of "click settings tab -> focus secondary search bar -> search formal settings name" for more regularly modified settings, it would instead be "focus primary search bar -> search bookmark name / have it queued up already -> click entry" and it takes you there. But parsing it out, the effort & complexity difference between those flows might be pretty marginal in practice. I will also work on resolving your other mentioned points and migrate this new component to the Composition API. |
… access operators
kommunarr
dismissed stale reviews from PikachuEXE and efb4f5ff-1298-471a-8973-3d47447115dc
via
6b9d7e6
kommunarr
force-pushed
the
feat/add-page-bookmarking
branch
from
6b9d7e6 to
476ea72
Compare
|
I have overlooked quite a few things when I approved this initially:
IMO things that I see the appeal for and look quite useful to bookmark are:
To summarize my thoughts, i understand the emulation of the browser bookmark system here but fail to see how this adds value when this is just saving stuff in a different way. Im all for giving users the freedom and flexibility to do stuff how they want it to do but most systems we have in place are far superior to this one. Yes FT is a YT desktop client with useful browser esque features but that doesnt mean we need to take all the browser features and put them in FT as some of them just dont make sense. At some point we have to make user familiar with stuff that we have in place instead of adding stuff just to make them feel more comfortable. |
|
If we don't have adequate consensus on the "separate system" of data idea, which is the main rebuttal to this that I find merit in, I can rework this PR to have it, e.g., be a "save page as corresponding type" button instead when there is already an existing more powerful system built-in for it (e.g., on a video, it represents adding / is it added to your Quick Bookmark playlist, on a channel, it represents subscribing / is it subscribed to by your current profile). I can see the issue with having it be "separate from" playlists, etc., but I still think the heterogeneity of access methods and the difficulty of searchability of saved content are salient and worthwhile problems to solve if nothing else. Thoughts? @efb4f5ff-1298-471a-8973-3d47447115dc @absidue |
|
This pull request in it's current state doesn't actually improve the searchability of saved content, it actually makes it worse as it introduces a second way to save various things and introduces a search mechanism for that specific way of saving content, so nothing changes about the searchability of the existing things. So you would have two ways of saving things that work completely differently. I would argue that it actually does the opposite of what you seem to be trying to achive, as you would then have to also remember where you saved what. Additionally reworking this pull request to do that would still not improve the situation, as you would have a magic button that does completely different things on different pages, which would just lead to more confusion. I am also confused that you are saying that you are struggling to use the prominent subscribe and quick bookmark buttons that you designed. |
|
I don't prefer to engage with people who talk so unnecessarily inconsiderately, so I apologize for asking for your opinion. You have largely misunderstood my suggestion and appear to be intentionally causing conflict by imagining things about my position and throwing in incendiary quips. In case your above post is further edited to bury these, I will mark these here for reference:
|
There was a problem hiding this comment.
Sorry for the late review, I can see this feature being useful for FreeTube power users.
From what I can tell, browsers don't add bookmarks to search suggestions when searching (at least by default) and only include search history/history in there. Is the plan to remove bookmarks from the search suggestions when search history is added and there's a separate page to view all bookmarks?
| import { isKeyboardEventKeyPrintableChar, isNullOrEmpty } from '../../helpers/strings' | ||
| import { getIconForRoute, isKeyboardEventKeyPrintableChar, isNullOrEmpty } from '../../helpers/strings' | ||
|
|
||
| const MAX_VISIBLE_LIST_ITEMS = 15 |
There was a problem hiding this comment.
I believe this would also limit the # of invidious/piped instances that are shown which might not be desirable
There was a problem hiding this comment.
Good catch, will look back into fixing this and your other comments once we can reach a cleaner team consensus on broader course of action as discussed here.
|
|
||
| const existingPageBookmark = computed(() => store.getters.getPageBookmarkWithRoute(router.currentRoute.fullPath)) | ||
|
|
||
| const appTitle = computed(() => store.getters.getAppTitle.replace(` - ${packageDetails.productName}`, '')) |
There was a problem hiding this comment.
jsdoc comment for appTitle:
/** @type {import('vue').ComputedRef} */
|
|
||
| const isBookmarkBeingCreated = computed(() => existingPageBookmark.value == null) | ||
|
|
||
| const pageBookmarks = computed(() => store.getters.getPageBookmarks) |
There was a problem hiding this comment.
jsdoc comment for pageBookmarks:
/** @type {import('vue').ComputedRef<any[]>} */
|
|
||
| const pageBookmarks = computed(() => store.getters.getPageBookmarks) | ||
|
|
||
| const duplicateNameCount = computed(() => { |
There was a problem hiding this comment.
jsdoc comment for duplicateNameCount:
/** @type {import('vue').ComputedRef} */
|
I've updated my comment to remove some of the more fiery parts, in the hopes that you'll consider the other parts of my message. |
|
I can think of >=2 things worth saving right now (which can't be saved):
I agree saved search criteria can be shown in search suggestion (if implemented well). But can also be shown in a separate page for better view (see criteria details, management etc.). Online playlist bookmark have no place in search criteria by default IMO. (Just create another page for it) I saw stuff like community post which I don't use so I don't comment on it |
|
Following some private discussion with some teammates, the main course of action is likely going to be implementing #1492 first, partially by adopting the search history DB machinery added here. After that, I wrote this set of options in terms of the team deciding the best course of action for page bookmarks: A. Keeping the PR concept mostly as-is
B. Breaking down aspects of the page bookmarking PR to potentially implement piecemeal as independent chunks:
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Page bookmarks
Pull Request Type
Related issue
closes #1505
closes #4040
#312
partial alternative for #4594
provides setup for #1492
Description
document.title, playlist name, or search query depending on routeScreenshots
Testing
General Settings > Enable Search Settingsis disabledTheme Settings > Match Top Bar with Main Colorenabled (secondary color theme is used as fill)Desktop
Additional context
Future PRs
search-history.dbentries withisBookmark: falseset. Edit: probably should just leverage the existing search cache as the data source & existing access methods, but we can reuse the UI patterns added in this PR. I can revert 392fa75, the commit I added specifically to better accommodate the possibility of a generic permanent search history through thesearch-history.db, if desired. It depends on whether we want to support a permanent search history and/or one with time-based expiry rules going forward.Aspects open for discussion
spellcheckon for the bookmark name insertionft-inputbecause we do it elsewhere for most of our inputs, even though it's a bit ugly. This one is debatable, but if I change it for this one, I'm also inclined to change it for other instances as well (e.g., User Playlist name insertion, profile name insertion).