Implemented bookmarking feature to add directories to places

[beef331]

Adds button to the header bar to add or remove a bookmark.
Bookmarks are stored inside a file at $XDG_CONFIG_HOME/Portfolio/bookmarks.

[tchx84]

hey @beef331 , thanks for working on this :) It would be a nice addition indeed.

I left some comments on how we could better integrate this, please check these comments and let me know if you have any questions.

[tchx84]

Maybe there's a way to reuse the already existent user-bookmarks-symbolic and handle the "filled" case with CSS?

[beef331]

I could be wrong, but I don't think it's possible to fill the svg's shape using CSS alone.

[beef331]

Given the mess made more sense to rebase, sorry if it causes any issues. Still uncertain how to pipe signals around this I for one don't like the indirection it hurts my head.

[tchx84]

Given the mess made more sense to rebase, sorry if it causes any issues. Still uncertain how to pipe signals around this I for one don't like the indirection it hurts my head.

No worries, I can still review it, but eventually we should fix this rebase, these older commits shouldn't show up.

[tchx84]

Looking much better :) But see my comments on how this model should be designed and used.

[tchx84]

how about _populate_initial_bookmarks ?

[tchx84]

Why do you need to check if it's in widget? If you're worried about duplicates, that's something you can deal with in the mode (e.g., using a set instead of a list). Same thing for available or unavailable bookmarks.

So this ideally look like:

for path in self._bookmarks.bookmarked:
    self._add_bookmark_place(path)

[beef331]

One may connect a device which has paths stored in their bookmarks, the check was to not repeat add to the collection. As on start up it can be come disjointed from the stored list as there may not be bookmarks accessible at the time of opening.

[tchx84]

Looking much better :)

Can you try adding a custom PortfolioBookmarkButton class and move that "icon update" logic inside it?

So, the window widget will:

1. Create the model (as it already does).
2. Create the button widget, and passes the model (missing).
3. Create the places widget, and passes the model (as it already does).
4. Update the button widget "path" property when the path changes (missing).

And, the button class will:

1. Handle the "clicked" event internally.
2. Handle the updating of the icon internally, in one place.

If done as described, the Window widget can simply do something like self._bookmark_button.path = path, and the button can internally handle all bookmark-related logic like talking to the model and update the icon, in one place.

[beef331]

Yea my uncertainty in how to do that is why it was not done 😄 I cannot find any reference in this codebase nor online how to properly do that.
I figured I just needed to do

class PortfolioBookmarkButton(Gtk.Button):
    __gtype_name__ = "PortfolioBookmarkButton"


    def __init(self):
        Gtk.Button.init(self)
        Gtk.connect("clicked", self._on_bookmark_toggle)

    def _on_bookmark_toggle(self, button):
        self._bookmarks.toggle_bookmark(button, path)
        if self._bookmarks.is_bookmarked(path):
            self.bookmark.props.icon_name = "bookmark-filled-symbolic"
        else:
            self.bookmark.props.icon_name = "bookmark-outline-symbolic"
            

Then inside the .ui change it to be PortfolioBookmarkButton instead of GtkButton, but that does not seem to work.

[tchx84]

Yea my uncertainty in how to do that is why it was not done 😄 I cannot find any reference in this codebase nor online how to properly do that.

No problem, for now just add an empty GtkBox to the window.ui file and call it something like bookmark_box, then, on window.py, create the PortfolioBookmarkButton and add it to that GtkBox.

EDIT: there are existing examples of this, look for "placeholder" in window.ui.

[tchx84]

@beef331 CI tests are failing, check the details. You need run pyflakes and black locally to fix these.

[beef331]

Are there any more changes you want to see?