Vieb bookmarks #391

yosoymau · 2022-07-01T16:32:01Z

yosoymau · 2022-07-01T16:32:45Z

This is what i have right now.
Now things are less convoluted than before as i moved everything to a new file, more organized. I have some things i'd like to discuss.

The `bmadd` command

What should we expect bmadd to do and how? Right now i have it set up like this:

Case 1:

bmadd
If the user sends the command with no arguments it creates a new bookmark, with the current page's url, title and name.

Case 2:

bmadd github my repos
When the user gives the command just a string of text, that string is considered to be the name, and the url and title are set to the current page's data.

Case 3:

bmadd name=github~title=main page of github~url=https://github.com
The user can give to the command a string like this, using this kind of style: option=value, and every option separated by a ~.

My concerns are:

Should the bookmarks be filled automatically if some piece of data is missing?
In case 3, should the missing data be filled automatically from the current page? Maybe this could be a setting?
My mind tells me that the url is kind of essential to the concept of what a bookmark even is, so i think this should be filled automatically if the user fails to specify one, but should any property of a bookmark be required?
If the user doesn't give a name for a bookmark? What should be the result? Right now it just gets the current page's title, but this seems bad to me.
I think the formatting of the string is not a very good one because both equal signs and tildes can be used in urls and titles, and that would mess things up, but i don't know what could be appropriate.
Also for the formatting, how should we accept list like properties? if the user wanted to give multiple tags to a bookmark, what would be a good format? Right now these are not supported.

Sorry if this are too many questions i just think we need to define this clearly before continuing much further with this.

Jelmerro

Thanks so much for starting work on this, looks fairly good, just a lot of small nitpick things 😉. Your overall approach and setup seem solid and correct. As for your other questions:

Yes, all info should have a fallback, even if empty in some cases. Pretty much everything besides the url should be considered optional in a bookmark.
Yes, if users don't want the info they can delete it later I think, unless you think a setting is needed as well.
Only the url is required, so if there is no page url, we need the user to supply one (only for the newtab page actually, but other pages are all fair as the url fallback).
Page title is fine to me, users can edit this later or set the title they want to.
I would imagine adding a url field specifically to the bmadd command not being that common, as usually users would like to simply add the current page, not some other url. Having said that, it should still be supported. I'm also not that big of a fan of how the command currently looks with lots of arguments, initially I thought of automatically guessing what each argument of the bmadd command would be for, but with so many different fields/options to a bookmark that seems not possible, so we do need this actually. I think the tilde is fine, it needs to be escaped in proper urls anyway, and is not stored as such in the bookmarks file, so even if users can't add it with the command when they are not on the page, they can still add it when they ARE on the page or by editing the bookmarksfile. Equal signs should be allowed in the options, you can split each part on the = and take the first part as the name, then join the other parts with = to get the remaining string with = intact.
Commas seem like a good option to me. Ideally the tags should follow simpler naming rules, you can use the specialchars variable of util.js to check for characters that should be blocked. Maybe choose the one that does allow spaces.

Hopefully you can continue with all these comments and answers, let me know if anything is still unclear. Finally please also refer to my later comments in #100 for a general description of how it should work.

app/renderer/bookmarks.js

app/renderer/settings.js

yosoymau · 2022-07-02T16:51:21Z

So now i've pushed an update where i started to add suggestions to the bookmark commands, it's still not working properly, i'm still not sure what's wrong, but i believe it's on the right track. Thanks for the feedback!

yosoymau · 2022-07-03T08:28:54Z

Hello @Jelmerro good day!
I've been thinking about the way that the command :bmload should work. I was working on the suggestions under the assumption that the user would load a bookmark referring to it by it's name, but then i remembered about the keywords and tags, and i'm not sure how these would fit here.

Can you explain what was the purpose that you intended for keywords, tags and names or how you expected them to work when you made the specifications on #100? This is so i can better understand what i should be aiming for, because right now i feel like i'm just kinda using the name as something like a keyword.

Also what do you think of this idea:

The user can do :bmload mytagand all the bookmarks with that tag get loaded.

Thanks!

app/renderer/bookmarks.js

Jelmerro · 2022-07-03T09:56:25Z

The tags and keywords should also be searchable using bmload, probably using the same syntax as bmadd, but i like the idea of just mentioning a tag and loading all of them. Not sure if the same should happen for keywords, but for tags that would be nice. The suggestions for bmload should help letting users know which bookmarks match their query, and load them all or just the one if selected.

The big difference of tags and keywords, should be that tags can be colored and have their own set of keywords, see #100 (comment). This should be all the more useful once there is a bookmarks page to visualize the bookmarks in the folders, where they are shown with these tags in their own colors.

yosoymau · 2022-07-04T14:10:17Z

I'm starting to work on the special page to manage bookmarks, and i ran into a roadblock that i haven't been able to solve. I'm trying to get the bookmark data from a function in renderer/bookmarks.js to preload/bookmarks.js. Now i found out these exist in different contexts, as far as i understand, these can't communicate directly. Is this right?

So my guess is that this should be done via ipcRenderer messages. I was looking around the code i dont have any idea on how i could get the data from renderer/bookmarks.js to maybe index.js (? not sure about this) and then get that sent to preload/bookmarks.js.

Any suggestion would be appreciated.

As for the bmload situation, i like the idea of using bmload with the same syntax as bmadd. I'll change what i have now and see how could i make this work in a nice way.

Jelmerro · 2022-07-04T21:13:26Z

You are correct, I would like to recommend the newtab page as a quick and easy example on how to do ipc-renderer communication: https://github.com/Jelmerro/Vieb/blob/master/app/preload/newtab.js, you can find the other side of things in a range of places, but the first point of action is here:

Vieb/app/renderer/tabs.js

Line 867 in 6d06e23

if (e.channel === "new-tab-info-request") {

. You do NOT need to go through main (unlike a lot of other things), as you don't need to interact with subframes or main-only APIs.

Basically, from renderer to preload/pages you can do webview.send and inside the preload you can do ipcRenderer.sendToHost to send it back to the renderer, which is received in the tabs file as an event on the webview. If you do ipcRenderer.send from the preload, you end up in main, but you won't need that in this case.

yosoymau · 2022-07-06T02:50:53Z

This was exactly what i needed, i'll be back when i get something working. Thanks a lot.

yosoymau · 2022-07-07T20:45:59Z

Now i've added the start of the special page for managing bookmarks, and this is what i got.
I don't know if this is the direction we were aiming for, right now it's kinda ugly, because it needs some css love, and things like tags or the editing functionality are not implemented yet.

My question is this: is this kind of solution good?
Right now this produces this page:

Should i keep going with this or should i change my approach?

Jelmerro · 2023-04-01T20:19:51Z

Hey @yosoymau I hope you are well, I was wondering if you were planning to continue work on this PR. Thanks for the work so far, let me know if you need help with anything!

yosoymau · 2023-06-06T17:24:22Z

Hello again, i'm back and i'll keep working on this, so expect to hear back from me soon.

I apologize for my absence and leaving this halfway for such a long time.

Jelmerro · 2023-06-06T20:29:45Z

Welcome back, glad you are willing to continue work on it, just let me know if there's anything you need help with. I'm a bit inactive for the last weeks as well, I will be back in a few.

Jelmerro

I went through the current code, most of it looks very good, just some small suggestions so far. I'm very happy with your commitment to build this feature over the past years, let me know if I can help with anything.

Jelmerro · 2023-08-07T18:10:26Z

app/preload/bookmarks.js

+}
+
+ipcRenderer.on("bookmark-data-response", (_, bookmarkData) => {
+    // Create folder structure


This comment seems redundant.

Jelmerro · 2023-08-07T18:10:44Z

app/preload/bookmarks.js

+"use strict"
+const {ipcRenderer} = require("electron")
+
+// Create tree structure to keep track of added folders.


This comment seems redundant.

Jelmerro · 2023-08-07T18:11:18Z

app/colors/default.css

@@ -350,6 +350,9 @@ body:not(#app) ::placeholder {color: var(--placeholder-text);}
 #downloadspage .misc {display: flex;margin: .5em;flex-direction: column;width: calc(100% - 1em);}
 #downloadspage .filelocation {cursor: pointer;}
 #downloadspage img {cursor: pointer;}
+/* Bookmarks  */


Bookmarks in lowercase here please.

Jelmerro · 2023-08-07T18:12:12Z

app/preload/bookmarks.js

+const update = () => {
+    ipcRenderer.sendToHost("bookmark-data-request")
+}


Suggested change

const update = () => {

ipcRenderer.sendToHost("bookmark-data-request")

}

const update = () => ipcRenderer.sendToHost("bookmark-data-request")

Jelmerro · 2023-08-07T18:13:12Z

app/preload/bookmarks.js

+        let currentCheckingFolder = tree
+        folderElements.forEach(e => {
+            // Ignore / because it already exists.
+            if (e !== "/") {


Maybe filter these before doing the forEach? And possibly make this into a function as this is intended quite a bit.

Jelmerro · 2023-08-07T18:18:05Z

app/renderer/bookmarks.js

+                {
+                    "id": t,
+                    "keywords": [],
+                    "name": ""
+                }
+            )


Suggested change

{

"id": t,

"keywords": [],

"name": ""

}

)

{"id": t, "keywords": [], "name": ""})

Jelmerro · 2023-08-07T18:18:42Z

app/renderer/bookmarks.js

+    // Check path format
+    // Check color hex values
+    // Check keywords and tags.
+    // Single words?


I presume these are still on the TODO list? Let me know if you are stuck and/or need help to implement this :)

Jelmerro · 2023-08-07T18:19:12Z

app/renderer/bookmarks.js

+    // Check keywords and tags.
+    // Single words?
+
+    if (badOptions.length !== 0) {


Suggested change

if (badOptions.length !== 0) {

if (badOptions.length) {

Jelmerro · 2023-08-07T18:19:53Z

app/renderer/suggest.js

+    [
+        "bmload",
+        "bmdel"
+    ].forEach(


Suggested change

[

"bmload",

"bmdel"

].forEach(

["bmload", "bmdel"].forEach(

Jelmerro · 2023-08-07T18:20:29Z

app/renderer/suggest.js

+                const {getBookmarkData,
+                    validBookmarkOptions} = require("./bookmarks")


Suggested change

const {getBookmarkData,

validBookmarkOptions} = require("./bookmarks")

const {

getBookmarkData, validBookmarkOptions

} = require("./bookmarks")

TiagoRCorreia · 2023-11-23T16:24:12Z

Still in development?

Jelmerro · 2023-11-23T16:54:05Z

I haven't seen @yosoymau around for a while, so this PR has been stale indeed. I am personally in no rush to get this implemented, but have outlined my proposal in the main ticket #100. I'm not going to rush @yosoymau to do it for us, they can take as long as they need to implement this huge feature, if at all, any contribution is optional and they shouldn't feel obliged to finish the PR, that's why it's still in draft as well. Feel free to pick up where this PR left and make a new one, or please wait for it to be done patiently.

yosoymau added 4 commits June 23, 2022 04:37

added settings and their initialization

0d13929

added missing bookmark settings

839ec03

moved implementation of bookmarks to new file

d6868b6

some cleaning and making things clearer

a791702

Jelmerro reviewed Jul 2, 2022

View reviewed changes

yosoymau added 2 commits July 2, 2022 10:16

small fixes, added suggestions to bm commands

bc22f77

small knitpicky fixes done

b5833df

yosoymau added 2 commits July 3, 2022 02:57

bmadd now supports list options for tags/keywords

defa3d8

Added fallback for all bmadd options

1e97dd0

Jelmerro reviewed Jul 3, 2022

View reviewed changes

app/renderer/bookmarks.js Outdated Show resolved Hide resolved

yosoymau added 4 commits July 3, 2022 07:37

now bookmarkFile gets created if missing

4208cad

Removed all from suggestbookmarks setting

b921463

fg and bg color values no longer added by default

27a53c2

Folders and tags are now being added when missing

a5ddb52

added bookmarks special page

c2441f9

Jelmerro mentioned this pull request Nov 8, 2022

about bookmark #441

Closed

Jelmerro mentioned this pull request May 24, 2023

Support of WebAuthn / Security Key #483

Closed

3 tasks

yosoymau added 2 commits July 5, 2023 06:02

added bmload, bmdel functionality, and small fixes

9fc66fd

fixed bm commands suggestions

c05ae09

yosoymau added 12 commits July 5, 2023 06:09

fixes to bm special page, and some formatting

d1443c9

Merge branch 'master' into bookmarks

747af03

now able to delete bm without input

ccbc599

renamed variables to more fitting names

063ba0c

bm suggestions now has two modes and actually work

b3f824b

changed some formatting and renamed variables

ce810a6

add: bm matching now works with multiple tag/kw

90e5f40

tag suggestion now supports multiple tags

2e4d553

keyword suggestions only give viable options

124f28c

Small string formatting fixes

eb3815e

suggestions improvements

debe35e

tag and keyword now supports "not" values

c67bc16

Jelmerro reviewed Aug 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vieb bookmarks #391

Vieb bookmarks #391

yosoymau commented Jul 1, 2022

yosoymau commented Jul 1, 2022 •

edited

Loading

Jelmerro left a comment

yosoymau commented Jul 2, 2022

yosoymau commented Jul 3, 2022

Jelmerro commented Jul 3, 2022

yosoymau commented Jul 4, 2022 •

edited

Loading

Jelmerro commented Jul 4, 2022

yosoymau commented Jul 6, 2022

yosoymau commented Jul 7, 2022 •

edited

Loading

Jelmerro commented Apr 1, 2023

yosoymau commented Jun 6, 2023

Jelmerro commented Jun 6, 2023

Jelmerro left a comment

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

Jelmerro Aug 7, 2023

TiagoRCorreia commented Nov 23, 2023

Jelmerro commented Nov 23, 2023

		const {getBookmarkData,
		validBookmarkOptions} = require("./bookmarks")

Vieb bookmarks #391

Are you sure you want to change the base?

Vieb bookmarks #391

Conversation

yosoymau commented Jul 1, 2022

yosoymau commented Jul 1, 2022 • edited Loading

The bmadd command

Case 1:

Case 2:

Case 3:

Jelmerro left a comment

Choose a reason for hiding this comment

yosoymau commented Jul 2, 2022

yosoymau commented Jul 3, 2022

Jelmerro commented Jul 3, 2022

yosoymau commented Jul 4, 2022 • edited Loading

Jelmerro commented Jul 4, 2022

yosoymau commented Jul 6, 2022

yosoymau commented Jul 7, 2022 • edited Loading

Jelmerro commented Apr 1, 2023

yosoymau commented Jun 6, 2023

Jelmerro commented Jun 6, 2023

Jelmerro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TiagoRCorreia commented Nov 23, 2023

Jelmerro commented Nov 23, 2023

yosoymau commented Jul 1, 2022 •

edited

Loading

The `bmadd` command

yosoymau commented Jul 4, 2022 •

edited

Loading

yosoymau commented Jul 7, 2022 •

edited

Loading