Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance(graph): #9943 Exclude from page graph #11059

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DrMagPie
Copy link

@DrMagPie DrMagPie commented Feb 29, 2024

Hello,

Trying to address #9943.

Implemented exclude logic for page graph and refactored a how exclude works in a global graph.

There are 2 open questions.

One: I assume that you do not want to set it as exclude always. So, we need to decide how to toggle it. I see two options here.

  1. Add it in the same way as the journal button is added.
    image
    image
    I'm not a fan of this option, it makes the UI more cluttered and I'm not sure it's the best course of action here.
  2. Add a configuration item that would change the behavior.
    Problem with this approach is that I have no idea how to do this. First time touching Clojure and did not find a good example in the code base :(

How do you want to proceed? Option 1 I have already implemented, just need to push :)

Two: This functionality has not been added to the block graph. I think it should be there too....
But for me block graph does not work at all. And I have no idea what it should look like.
So I haven't touched it at all.
image

Side note, it includes built-in pages (TODO,DOING) to the graph... is that supposed to be like that?

@CLAassistant
Copy link

CLAassistant commented Feb 29, 2024

CLA assistant check
All committers have signed the CLA.

@DrMagPie
Copy link
Author

DrMagPie commented Feb 29, 2024

Did some more digging.

And perhaps it is worth adding same modal as in global graph, and unifying all filtering under one function.
image

Then that function would look like this

(defn exclude-from-graph
  [nodes links {:keys [journal? orphan-pages? builtin-pages? excluded-pages?]}]
  (let  [aliases (if-not excluded-pages?
                   (->> nodes
                        (mapcat (fn [node]
                                  (when (=  true (:exclude-from-graph-view (:block/properties node)))
                                    (:block/alias node))))
                        (remove nil?)
                        (map :db/id))
                   ())
         build-in-pages (if-not builtin-pages?
                          (set (map string/lower-case default-db/built-in-pages-names))
                          ())
         linked (if-not orphan-pages? (set (flatten links)) ())]
    (cond->> nodes
      (not journal?)
      (remove :block/journal?)
      (not orphan-pages?)
      (filter #(contains? linked (string/lower-case (:block/name %))))
      (not builtin-pages?)
      (remove (fn [node] (contains? build-in-pages (string/lower-case (:block/name node)))))
      (not excluded-pages?)
      (remove (fn [node]
                (or
                 (not (nil? (some #{(:db/id node)} aliases)))
                 (= true (:exclude-from-graph-view (:block/properties node)))))))))

This way it does look cleaner
image

And functionality wise it is identical to the global graph.
image

A question on an unrelated topic, what are other-pages-links for?

other-pages (->> (concat (map first ref-pages)
(map first mentioned-pages))
(remove nil?)
(set))
other-pages-links (mapcat
(fn [page]
(let [ref-pages (-> (map first (db/get-page-referenced-pages repo page))
(set)
(set/intersection other-pages))
mentioned-pages (-> (map first (db/get-pages-that-mentioned-page repo page show-journal))
(set)
(set/intersection other-pages))]
(concat
(map (fn [p] [page p]) ref-pages)
(map (fn [p] [p page]) mentioned-pages))))
other-pages)

While I was poking at the code I have removed them, and it did not affect behavior of the graph...

@andelf andelf self-requested a review March 6, 2024 08:05
@andelf andelf added the graph Graph view label Mar 6, 2024
@DrMagPie
Copy link
Author

Hi @andelf

What do you think about my proposal in the last comment?
Should I proceed with it and push the implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graph Graph view
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants