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

Implement a backlinks query for a given Zettel ID #216

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

pjones
Copy link
Contributor

@pjones pjones commented Jun 4, 2020

Work in progress: do not merge.

Will be useful for editors to show a "nearby zettels" view. See
felko/neuron-mode#30 for more details.

@srid
Copy link
Owner

srid commented Jun 6, 2020

Something to think of: are we looking for non-folgezettel backlinks (the list displayed below the zettel), or also uplinks (the tree displayed above the zettel)?

@pjones
Copy link
Contributor Author

pjones commented Jun 10, 2020

Something to think of: are we looking for non-folgezettel backlinks (the list displayed below the zettel), or also uplinks (the tree displayed above the zettel)?

What I'm personally looking for is the ability to get a list of all zettels that link to the one that I'm editing. This would include ?cf links as well. I'm not sure if this list would be called backlinks or uplinks or what.

@srid
Copy link
Owner

srid commented Jun 10, 2020

Then perhaps:

GraphQuery_BacklinksOf :: 
  -- | If a Just value, get only backlinks with that connection type.
  Maybe Connection -> 
  ZettelID -> 
  GraphQuery [(Connection, Zettel)]

And pass Nothing as first argument to get all backlinks regardless of connection type.

I'm not sure if this list would be called backlinks or uplinks or what.

They are all backlinks. An 'uplink' is a kind of backlinks whose connection type = Folgezettel (i.e., non-cf). Uplinks are what are displayed on top of the zettel, as an inverted tree. If using this in editor support, I'd find it useful to be able to navigate by uplinks and downlinks (again, non-cf).

@NullSense
Copy link

@pjones pjones force-pushed the pjones/backlinks branch from 36773ff to 83c759a Compare June 19, 2020 22:58
@pjones
Copy link
Contributor Author

pjones commented Jun 19, 2020

@srid I have the --backlinks-of query working. If you approve of the implementation I will also implement --folgezettel-of as well.

What would the query flag be for the OrdinaryConnection connection?

Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comments in your implementation. Based on current terminology we could go for the following cli spec:

  • --backlinks-of: include all backlinks (folgezettel & ordinary)
  • --uplinks-of: include only folgezettel backlinks (aka. 'uplinks')
  • --cf-backlinks-of: include only ordinary backlinks

Is there a reason to implement the third option in the list? I would think that just the first two would be sufficient.

neuron/src/lib/Neuron/Zettelkasten/Graph.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Query.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Query.hs Outdated Show resolved Hide resolved
neuron/src/lib/Neuron/Zettelkasten/Query.hs Show resolved Hide resolved
@pjones pjones force-pushed the pjones/backlinks branch from 83c759a to 7a939e6 Compare June 24, 2020 01:57
@pjones pjones requested a review from srid June 24, 2020 02:06
@pjones pjones marked this pull request as draft June 24, 2020 02:07
Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjones This looks good! Just one main change, and we are good to go with the merge.

neuron/src/app/Neuron/CLI/Types.hs Outdated Show resolved Hide resolved
neuron/src/app/Neuron/CLI/Types.hs Outdated Show resolved Hide resolved
Comment on lines 54 to 57
let includeConn = maybe isJust (const (== conn)) conn
g' = LAM.transpose $ G.getGraph $ G.induceOnEdge includeConn g
ns = Map.toList $ Map.findWithDefault mempty (G.vertexID z) $ LAM.adjacencyMap g'
in mapMaybe (\(v, e) -> (,) <$> e <*> G.findVertex v g) ns
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation should be moved to Data.Graph.Labelled.Algorithm. You can just put them in the existing preSetWithEdgeLabel function.

Also, you should use getVertex here instead of findVertex. The former works with the guarantee that the vertex exists -- which is the case here -- and doesn't have to deal with Maybe. This way you also don't have to ignore the error via the Applicative lift.

Aside: Maybe we should just pass the filter function directly -- i.e., (Maybe Connection -> Bool) instead Maybe Connection -- but it is okay if that's not done in this PR>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move about half of the implementation into Data.Graph.Labelled.Algorithm due to preSetWithEdgeLabel being polymorphic in the edge type.

In backlinks the edge type is specialized to Maybe Connection which is why I post process the postSet by removing Maybe from the edges. The other issue with moving more of the code into preSetWithEdgeLabel is that LAM.traspose imposes more constraints on the edge type than currently exist. For example, e would need to be an instance of Monoid and Ord.

How does that sound to you?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preSetWithEdgeLabel can just take those extra constraints on e. If you pass (e -> Bool) instead of Maybe Connection (as the filter) to it, the function can remain polymorphic - and you can then move all of the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the logic has been moved into preSetWithEdgeLabel. Please let me know what you think.

Copy link
Owner

@srid srid Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjones Almost there. The backlinks function should take (Maybe Connection -> Bool) instead of Maybe Connection - just to avoid potential confusion.

(Maybe Connection is used as the edge type only because the algebraic-graphs library relies on it to be a monoid - where the empty value signifies "no connection"; using the same type for the filter can be confusing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. The latest commit has this change.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Once you mark it non-WIP, I'll merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@pjones pjones force-pushed the pjones/backlinks branch 3 times, most recently from 0d0dd78 to 5b0e296 Compare June 25, 2020 16:10
@srid srid added this to the 0.6 milestone Jun 25, 2020
Useful for editors to show a "nearby zettels" view.  See
felko/neuron-mode#30 for an example.
@pjones pjones force-pushed the pjones/backlinks branch from 5b0e296 to 476097a Compare June 25, 2020 17:45
@pjones pjones changed the title WIP: Implement a backlinks query for a given Zettel ID Implement a backlinks query for a given Zettel ID Jun 25, 2020
@pjones pjones marked this pull request as ready for review June 25, 2020 17:46
@srid srid merged commit b311e9a into srid:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants