logseq
logseq copied to clipboard
logseq
fix: make query show blocks that refer to a page, rather than to one #7183
Thank you! I'm happy to review :) Feel free to ping me when this PR is ready
And hope for more detailed description of the changing, as the concepts in query are a bit abstract
Let me know what needs to be clarified from the business-logic side of things.
In short, when we query for a page we get all the blocks that referred to that page, no matter how they named such a ref (using an alias name or a page name - no regard).
Hey @cnrpman, how to get this lint error of our backs?
Humm, maybe @cldwalker can have a look on this error
On the side, I'm not proud of the solution at all, it's horrendously complex. :D Taking a look at how complex it is to express "all blocks that ref to a block" with our current data model raises a question - would we like to have a more fit data model?
It seems the root cause of complexity is in that we treat names as IDs, whereas they are aliases - the same thing can be called by many names, per person (i.e., names are personal dictionaries (e.g., on a programming example, clojure.core/reduce could be called by it's full name, reduce, foldl etc. - names are user-level, but the artifact they refer to is the same - we want to capture that user refers to something, by it's id, and we may capture on the side how he preferred to name that thingie)).
Would love to hear your thoughts on it, guys. @cnrpman @cldwalker @tiensonqin
@andrewzhurov Thanks for trying to fix the issue but the proposed rule is too complex to be used with other rules. We already have difficulty using multiple
propertyandpage-propertyrules and those are much simpler rules to maintain. Once #7333 lands you could see if (re)using a rule likealiascould help. If you do try this again, please write a test or two to ensure the behavior will not regress
Hey @logseq-cldwalker, thanks for the pointer.
I've taken a look at the proposed :alias rule, it seems to not be fit for the use-case.
What we want from :page-ref rule is to return all blocks that ref to a page, with no regard to name they use for it.
But since reference to a page made via an alias is captured as reference to that alias, we need to find all blocks that represent 'reference to a page', which is #{page alias_1 ... alias_n}, and find all blocks that reference anybody from that set..
It is complex, I agree, however I think that comlpexity creeped in at the level of data model, and it can't be escaped from at the level of rules. There is a ton of issues regarding aliases, they are about UX expectations, of course, not the data model. But it seems that aliases are modeled in a way that makes it hard to satisfy those UX expectations. Perhaps remodeling them to be more closely aligned with UX expectations would make data model and surrounding code less complex, hence less bug-prone and maintaince demanding. Can be made as simple as referencing a page directly, with no regard of name used, and storing how this ref is named locally, on the entity that referenced. Keen to hear your thoughts on this.
I'm all up to have tests, wanted to write some, what got my enthusiasm down is the fact that I didn't manage to get Cider support on test files, as they are meant to be executed in :node-test environment and Cider default repl is in :browser environment, and getting another repl with :node-test env gave some errors. Is there a way to get Cider support on test files? Not having it makes test writing a huge effort, which is a bummer. One way is to have :browser env for tests and run them via kaocha, although it'd be a fair bit of effort to implement, I imagine.
It is complex, I agree, however I think that comlpexity creeped in at the level of data model, and it can't be escaped from at the level of rules. There is a ton of issues regarding aliases, they are about UX expectations, of course, not the data model. But it seems that aliases are modeled in a way that makes it hard to satisfy those UX expectations. Perhaps remodeling them to be more closely aligned with UX expectations would make data model and surrounding code less complex, hence less bug-prone and maintaince demanding. Can be made as simple as referencing a page directly, with no regard of name used, and storing how this ref is named locally, on the entity that referenced. Keen to hear your thoughts on this.
Number of issues is not a useful signal for refactoring on a large, fast moving project like ours. We refactored filenames less than 2 months ago and we have more issues with it than alias.
I'm all up to have tests, wanted to write some, what got my enthusiasm down is the fact that I didn't manage to get Cider support on test files, as they are meant to be executed in :node-test environment and Cider default repl is in :browser environment, and getting another repl with :node-test env gave some errors.
I don't use emacs but we have instructions that take less than a minute to try. Would recommend starting there with tests. Tests would be required for adding alias support to queries
I've taken a look at the proposed :alias rule, it seems to not be fit for the use-case.
Sorry to hear it won't be useful for this. Unfortunately adding alias support to our simple queries is not a priority right now so I don't have time to dig into this more. If you are interested in this, I would encourage you to seek a solution that relies on a simple, reusable rule(s). Simple and reusable are important traits as we're looking for an approach that can then be applied later to other query filters that don't have alias support e.g. page, property and page-property
Hi @logseq-cldwalker,
thanks for your reply.
Number of issues is not a useful signal for refactoring on a large, fast moving project like ours. We refactored filenames less than 2 months ago and we have more issues with it than alias.
If a project wants to be fast-moving it needs to be lean on accidental complexity. It seems to me that current design of aliases adds some, issues is a good indicator of it, as people raise that current UX of aliases does not match expected UX, and current UX almost directly comes from the current data model, hence current data model does not match expected data model. Remodeling it would spare us the need of having a code layer that adjust data model to the expected UX.
Issues such as: Page/block name queries ignore aliases #7183 Hierarchy lists both aliased and original pages #6986 Publishing: aliased page does not pick up publish attribute #5280 Page aliases are unaffected by the page property exclude-from-graph-view #6213 Aliases are duplicating in the graph view and the searching result #4709
won't be there in the first place if we are to model aliases according to their expected UX.
I don't use emacs but we have instructions that take less than a minute to try. Would recommend starting there with tests. Tests would be required for adding alias support to queries
Having tests is a must, I agree. Thanks for the link, it's a fine workflow to run tests, what bugs me is lack of my IDE support on test files (jump to definition etc.), that makes test-writing less joyful, does your IDE have support? I'm asking to find out if that's a problem that affects more people than me. If so, we could switch to running tests in the same :browser environment as the rest of the app, e.g., via Karma (I've been mistakenly referring to Karma as kaocha before, sorry for confusion, I mean to propose for tests to target Karma). I imagine that would be of little priority to your team, as there are more important tasks to take on, but if that is something doable within a week then I could take a look into it.
have time to dig into this more. If you are interested in this, I would encourage you to seek a solution that relies on a simple, reusable rule(s). Simple and reusable are important traits as we're looking for an approach that can then be applied later to other query filters that don't have alias support e.g. page, property and page-property
That's a good mention, thanks, I see now how support for aliases can be made as a reusable rule, pluggable in all other rules that work with pages. However, carrying on in this direction makes me wary, as it would spread accidental complexity coming from an inappropriate alias data model even further to codebase. I would really hope we could not have these problems to solve in the first place. I get it that it's of low priority, perhaps I could look into it as well. That would require, however, a bit of your attention to validate data model I come up with. Could you let me know whether that would be alright with you?
However, carrying on in this direction makes me wary, as it would spread accidental complexity coming from an inappropriate alias data model even further to codebase. I would really hope we could not have these problems to solve in the first place. I get it that it's of low priority, perhaps I could look into it as well. That would require, however, a bit of your attention to validate data model I come up with. Could you let me know whether that would be alright with you?
You're welcome to explore new data models but I won't have time to help with brainstorming, just review of a complete PR. Changing our data model requires changes in a number of places and breaks users who use advanced queries. I haven't heard convincing arguments to incur that user pain but maybe your proposal could surprise us. My recommendation is a small change that can be reused across query filters, with tests to demonstrate the desired behavior works.
I'll put this PR on pause and create another with an adjustment to datamodel. It seems that it can be made as uninvasive as linking to referenced pages directly, instead of their alias nodes. This will solve the forementioned issues when linking from now on. Existing refs to aliases would need to be migrated. Could somebody hint how it can done in Logseq? @cnrpman
@andrewzhurov Sorry but I'm not familiar with this part of code, and have no idea about how to do this easily..
Sounds need a proxy layer to handle the :block/alias :block/refs and :block/path-refs in the UX
Agree with @logseq-cldwalker that it requires changes in a number of places. But it shouldn't be a big breaking change to users as unifying the alias is more reasonable.
@andrewzhurov Sorry but I'm not familiar with this part of code, and have no idea about how to do this easily.. Sounds need a proxy layer to handle the :block/alias :block/refs and :block/path-refs in the UX
No worries about the details, I'll figure it out, I'm wondering how migrations are usually written for logseq / is there a standard way for it
Agree with @logseq-cldwalker that it requires changes in a number of places.
Yup, looks like there is a lot of places to tweak, although perhaps it'll make them simpler, so the more - the better.=)
@andrewzhurov Thanks! For breaking changes, usually we provide config option to roll-back to the previous behavior.
I'm wondering how migrations are usually written for logseq
@andrewzhurov For a change this big it wouldn't be a migration but like @cnrpman said, a config option to opt into the new model. Will close this PR since it is WIP and you're going in a different direction. I would recommend looking up all the places that use aliases in order to come up with a solution that handles them all
Hey, thanks guys, I get that this new behaviour better be togglable. However, if it's toggled on I'd expect it to affect all data and not only new one created from then on, hence the need to migrate. Migration could be run when user toggles it on, and another when user toggles off. Although we could re-index, deriving data in whatever datamodel user likes it most, huh.. that seems way simpler I guess I answered my own question.=)