Should hitting alt-s call addPageBookmark, not toggleBookmark?

[stevegt]

I'm wondering if hitting alt-s (or whatever key is bound to the "Favorite Page" shortcut) should simply add a bookmark. Right now (b9ee57f41759a8285d87843ead9846a937892980) it toggles instead. This means that, if a page has already been marked, a user hitting alt-s on a page they want to mark has the effect of removing the mark. This can lead to unintentional loss of bookmarks.

In my own case, I've found myself having to hit alt-s multiple times each time, watching the ribbon pop up so I can see whether the little heart icon is filled or not -- this is bad for usability. I'm sure I've removed marks that I meant to keep, and the worst part is that I'll likely never know. (I feel like there's some sort of Hippocratic-like oath a true memex should have -- "first, don't lose data". At least this one is an easy fix.)

I'd be happy to send a pull request to turn off the toggling and just call addPageBookmark -- does anyone have any objections?

[stevegt]

I did a little more digging:

While the toggle feature probably made sense at the time it was implemented, it makes less sense now that we need to be able to quickly and easily mark an interesting page for indexing -- I've found myself hitting the bookmark key to accomplish this. The ability to easily index pages manually became critical when auto-indexing was disabled in 41fadff546c495c918f642ce6fe7cb95b278ff51. That was only three months ago, so I'm not surprised if I'm the first person to raise this as an issue.

The decision to make alt-s be a toggle apparently happened in https://github.com/WorldBrain/Memex/issues/273#issuecomment-364881231:

[...] i would expect that it would be a toggle, also sending a notification for the page being unbookmarked.

It's interesting to note that the "notification" part either was not implemented at the time, or has since broken. Regardless, I'd recommend that a keystroke be idempotent, always producing the same result. That enables the users' muscle memory to drive it, rather than expecting the user to look at a particular part of the screen for a notification of what the keystroke actually did after hitting it. Another alternative would be making the notification be a dialog such that the user has to click e.g. cancel or are you sure? -- that would be maddening, so I'd avoid that.

[stevegt]

Another alternative would be to add yet another key binding, call it the "Index page" shortcut, that would cause indexing to happen, without adding a bookmark or requiring the user to enter a tag or collection name. This is probably the right way to deal with the disabled auto-indexing in the first place. This would decrease the use of alt-s and lessen the likelihood of lost marks. I've opened #1142 for discussion.

Either way, I'd still make the bookmark action be idempotent as described here. If there are no objections, I'm still willing to provide a pull request for this.