minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.23k stars 103 forks source link

consult-line + embark occur, candidates have no actions associated #29

Closed mpenet closed 3 years ago

mpenet commented 3 years ago

Hi,

First of all, thanks for the package, it's quite nice.

When I try to use embark-occur with consult-line candidates I get a new buffer with the candidates, however they are not "clickable" and do not allow to jump to the results. I tried the same with the original code from the selectrum wiki and the behavior is the same.

You can see the concerned config portion here https://github.com/mpenet/emax/blob/master/init.el#L327-L352.

I have the feeling we might need a custom embark-candidate-collectors fn

minad commented 3 years ago

Thank you for the report. @oantolin and I are currently discussing in how we can make embark and consult work together nicely, right now planning to reduce overlap between the libraries (https://github.com/oantolin/embark/issues/41). Ideally we won't introduce any hard dependencies between the libraries

mpenet commented 3 years ago

That's good news.

On a slightly unrelated note, is there any plan to add a counsel-rg equivalent to consult? That's the only thing left I use from ivy/counsel and I am quite confident it's something very popular among counsel users. There's a recipe on selectrum wiki to do it but I think it is outdated.

On Thu, Dec 3, 2020, 11:00 Daniel Mendler notifications@github.com wrote:

Thank you for the report. @oantolin https://github.com/oantolin and I are currently discussing in how we can make embark and consult work together nicely, right now planning to reduce overlap between the libraries (oantolin/embark#41 https://github.com/oantolin/embark/issues/41). Ideally we won't introduce any hard dependencies between the libraries. But the libraries should work together nicely.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/minad/consult/issues/29#issuecomment-737814024, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZ7FTWHN3LE4DVQ6SYQ2LSS5OTHANCNFSM4ULU33MQ .

minad commented 3 years ago

Please add commands you miss to the command wishlist if they are not already there. I have to look into consult-grep/consult-rg at some point I guess. But I would also like to hear about the overlap between the grep/rg package and the corresponding counsel command. Are you exclusively using the counsel-rg command then or do you also use the rg package? The goal for me is also to reduce overlap with other packages. If there is another package which provides a strictly better version in most cases I think it may be better to not add anything here. If you take a look at the commands currently added - these commands are all related to basic Emacs functionality and thus uncontroversial from my pov. But when I look at your config, you seem only to be interested in the search function, so probably the needs vary greatly.

mpenet commented 3 years ago

Sure, will do.

The various rg package(s) do not provide live candidates preview, they do provide more options tho. counsel-rg does show results live but it is usually used more like a quick navigation/jump utility than a full blown search tool I think. Paired with occur it's quite powerful. So far I couldn't find a decent counsel-rg replacement.

On Thu, Dec 3, 2020, 11:34 Daniel Mendler notifications@github.com wrote:

Please add commands you miss to the command wishlist if they are not already there. I have to look into consult-grep/consult-rg at some point I guess. But I would also like to hear about the overlap between the grep/rg package and the corresponding counsel command. Are you exclusively using the counsel-rg command then or do you also use the rg package? The goal for me is also to reduce overlap with other packages. If there is another package which provides a strictly better version in most cases I think it may be better to not add anything here. If you take a look at the commands currently added - these commands are all related to basic Emacs functionality and thus uncontroversial from my pov. But when I look at your config, you seem only to be interested in the search function, so probably the needs vary greatly.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/minad/consult/issues/29#issuecomment-737855667, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZ7FVGCLJOSCI7TLSUUQ3SS5SVBANCNFSM4ULU33MQ .

minad commented 3 years ago

Ok, I expected the rg package also provides live preview somehow. Maybe it would be worth to consider adding support there. Has this been asked before? This does not exclude adding support here.

Another problem with counsel-rg is that the the candidate list is generated dynamically after invoking rg. This is a bit of a pain point in the consult project here, since the Emacs API is a bit complicated if you need that functionality. Ivy/Counsel can easily solve this, since they just bake in the needed feature into Ivy. There is also #9 where I am thinking about dynamic candidate lists with regards to consult-buffer and consult-match (not yet there). Something like consult-rg will have the same issues.

mpenet commented 3 years ago

Ok, I expected the rg package also provides live preview somehow. Maybe it would be worth to consider adding support there. Has this been asked before? This does not exclude adding support here.

I saw that mentioned a few times on r/emacs at the very least. A quick search showed that one on top: https://www.reddit.com/r/emacs/comments/gixh12/quickly_grepping_project_in_vanilla_fashion/

I wouldn't be surprised if it's not the most popular feature from counsel after swiper. Personally I use counsel-rg all the time, it's really powerful.

minad commented 3 years ago

Ok. Thank you for the link. Interestingly, in the reddit thread both @clemera and @oantolin (with whom I am interacting a lot regarding selectrum/embark/consult) reacted, also mentioning that completing-read is problematic with regards to providing something like dynamic candidate sets. There is no strict rule to not include selectrum specific code in consult (e.g. consult-buffer has a selectrum specific implementation as of now), but I still want to push it a bit to avoid completion system specifics. These things are being sorted out I guess, but slowly. In the meantime a selectrum-specific version could be added.

I see the appeal of counsel-rg. When I tried the rg package recently it had problems with the rg version I had installed on my stable debian. So I guess there are still many things in flux. These are a bit the kind of things preventing me from quickly implementing something like this...

mpenet commented 3 years ago

In the meantime a selectrum-specific version could be added.

I think that should be the right path too.

I also found that one: https://github.com/raxod502/selectrum/issues/164 (selectrum has a few other mentions)

minad commented 3 years ago

I think that should be the right path too.

Generally, I don't think it is the right path, it is only a stop-gap measure. I added consult-rg to the wishlist for now. If you would want to have this feature sooner than later, I would like to encourage you to create a PR. But please note that the PR should be a little more polished than the wiki versions, adhering to the style of the project, bugs sorted out, robustly written etc. Otherwise it is no point in having it here, since it is better to have something which does not fall apart the next time something is changed somewhere. Furthermore the question should be sorted out if there is any possibility to provide a version based on completing-read/read-string, even if it is not as powerful, just in order to have some kind of graceful fallback in case selectrum is not available.

oantolin commented 3 years ago

For a potential consult-rg, maybe this ungodly hack would be good enough: async-completing-read. (It grew out of the reddit discussion @minad linked to).

As for the original issue with consult-line, doesn't the embark occur buffer at least let you save lines on the kill ring? That action should definitely work. Of course, ideally pressing RET would jump to the line. I'll look into it now that I'm working with @minad to ensure proper cooperation between embark and consult, but fixing this might not happen extremely soon. (I've had my own version of consult-line for a while, and I've been meaning to make it work properly with embark-occur for a long time, this issue will finally give me the impetus to do it --or rather, to drop my function in favor of consult and then fix the issue).

minad commented 3 years ago

@oantolin Interesting to hear about this async-completing-read package. I will take a look, but this is low priority right now since it just concerns some commands.

Regarding the original issue of consult-line - I am also using some ugly hack. I am prepending the lines with the line numbers encoded as unicode characters in the private use planes for deduplication of equal lines. You don't do deduplication, do you? Instead I could also prefix simply with the line number, but then the line number would be part of the candidate string and this is what I wanted to avoid here.

oantolin commented 3 years ago

You don't do deduplication, do you?

No, good catch, I don't. My function takes you to the first line that's identical to the one you chose, which, of course, may not be the completion candidate you actually selected.

minad commented 3 years ago

I had it wrong at first too I think and someone told me, maybe @clemera or @okamsn. Originally I ported over the version from the selectrum wiki where the line number was returned via a property. Then you don't have the duplication problem. Unfortunately using properties for returning associated data does not work with completing-read. Therefore the hack I am using right now. Please let me know if you have a better idea.

s-kostyaev commented 3 years ago

Please let me know if you have a better idea.

You can use relative numbers of identical candidates instead of real line numbers, i.e. first invocation of current candidate, second one etc. This solution will work even if line numbers will be changed.

minad commented 3 years ago

You can use relative numbers of identical candidates instead of real line numbers, i.e. first invocation of current candidate, second one etc. This solution will work even if line numbers will be changed.

I don't see how that helps. The problem is not if line numbers change. The problem is that I have to add a prefix to a string in order to disambiguate. The prefix however should not be used for the matching. By using unicode characters in the private plane I make that somehow improbable but not impossible. Maybe if someone searches for smileys or something we will get false matches. I don't know what could be in those planes. This unicode prefix is not visible since I propertize it with 'display.

oantolin commented 3 years ago

Hey, @mpenet I think the issue with consut-line needed to be solved on the embark side, so I pushed commit oantolin/embark@bd071c45a43a77ab87f30bc7a89f1cded328a836. Please test.

(I think this is the type of cooperation without explicit dependency that @minad encourages: the commit above gives consult-line the same special treatmente embark gives imenu, but it only mentions the symbol consult-line, not the actual function, and is completely harmless for people not using consult.)

oantolin commented 3 years ago

One issue with embark & consult-line, is that the insert and save actions will use the line as consult-line hands it over: including the leading unicode-character line number. I guess that's not too bad, just delete it after inserting or yanking. It is just a single character. :shrug:

minad commented 3 years ago

@oantolin If we can agree on a format of this prefix, you could also strip it in embark. Right now I am using this unicode encoding. I could also add another separator character after that to ease the removal. I am not sure if you want to do something like this. The line format is kind of a hack unfortunately. But since we can define our own line-category for this, I think that is okay.

oantolin commented 3 years ago

@minad I don't think the general embark insert or save actions should ever strip a prefix. What I suggest is that (1) you go ahead with adding a line category in consult, (2) I add an embark-line-map for the line category with actions that do strip the prefix, and with actions that, say, insert or save just the line number. How does that sound?

oantolin commented 3 years ago

I just want to add that though the line format is a hack, I haven't been able to come up with a better idea, so I'm happy with it: the consequences of the hack can be dealt with.

oantolin commented 3 years ago

Oh, I hadn't realized that for line number higher than 65534 you emit several prefix characters. I guess that's OK.

minad commented 3 years ago

@oantolin What about introducing the concept of a candidate transformer? Could something like this be added to embark? Another less general idea - I could propertize the candidate prefix with 'candidate-prefix and this gets ignored by embark. Sure, I can always define my own custom actions which do that as you suggested as an alternative. I guess we don't have to solve this now, but I am pretty sure we will see people coming and complaining about the tofus.

oantolin commented 3 years ago

What about introducing the concept of a candidate transformer? Could something like this be added to embark?

Yes, and it would solve the problem for the current insert and save actions. But if we still also want a "save line number" action something else would be required. I just made that action up, but I think it is something someone might want: say you want to mention a line number in an email, you pop over to the buffer containing the line you want to mention, run consult-line to find the line and then run the "save line number" action, pop back to your email and yank the number. That sounds so satisfying I want to do it right now. :P

Another less general idea - I could propertize the candidate prefix with 'candidate-prefix and this gets ignored by embark.

I prefer the candidate transformer idea.

Sure, I can always define my own custom actions which do that as you suggested as an alternative. I guess we don't have to solve this now, but I am pretty sure we will see people coming and complaining about the tofus.

I was suggesting writing these actions and putting them in embark. They would be a in keymap that is only used for the category "line", and so wouldn't do anything for people who don't use consult. I think that's fine.

I think maybe just writing these actions, adding them to embark and not bothering yet with candidate transformers is the simplest idea.

minad commented 3 years ago

I was suggesting writing these actions and putting them in embark. They would be a in keymap that is only used for the category "line", and so wouldn't do anything for people who don't use consult. I think that's fine.

Sure you can do that in embark, but it would be great if we could keep the consult specifics to a minimum. In particular since this line prefix thing is an implementation detail. If we introduce this 'candidate-prefix property, this would be a bit more decoupled.

If you want to have different actions depending on the candidate and some of the actions even accessing the line number prefix, it seems to me there is no way around writing specific actions. However, note that the line number could be extracted from the marker of the candidate. The candidate alist is of the form (("unicodeprefix-string" . marker) ...). But if the candidate is already transformed such that the prefix has been stripped you cannot lookup the marker. I don't know...

mpenet commented 3 years ago

Hey, @mpenet I think the issue with consut-line needed to be solved on the embark side, so I pushed commit oantolin/embark@bd071c4. Please test.

It's working fine thanks!

oantolin commented 3 years ago

Now that consult has more category metadata, embark-occur doesn't need to give special treatment to consult-line specifically, and now consult-outline works with embark-occur as well.

minad commented 3 years ago

Consult line, outline and mark should all work with occur. But mark has a different category. Should I change sth there?

oantolin commented 3 years ago

I just made an even better fix for this problem! oantolin/embark@2d6fa05317e2ca8304552ff7a5671aef2dd74871 There are no longer any special cases in embark for this, not for the line category and not for imenu. So consult-mark and other future commands like it now work out of the box with embark-occur.

Even so, maybe it would be better if consult-mark reported category line: it's candidates really are lines, the lines containing markers. Why does it report mark? Was there some use case you had in mind which would be easier if consult-mark and consult-line report different categories?

minad commented 3 years ago

The difference is that the marks are not at the beginning of the line. But still the whole candidates are lines, so I will change this.