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

M-i in counsel-compile fails due to propagated 'cmd property #2040

Open
stsquad opened this issue Apr 24, 2019 · 6 comments
Open

M-i in counsel-compile fails due to propagated 'cmd property #2040

stsquad opened this issue Apr 24, 2019 · 6 comments

Comments

@stsquad
Copy link
Contributor

stsquad commented Apr 24, 2019

https://github.com/abo-abo/swiper/blob/0e62f0d1f61b825ca5eb4b55e47ecb37b3e2834e/counsel.el#L5379+

I've noticed this recently when trying to use compile targets not in the generated list. We fall-over because we look for the cmd property and unless the new target is exactly the same length it will get it wrong. I've thought of something like the following:

(if (get-char-property 0 'cmd cmd)
    (setq cmd (substring-no-properties
               cmd 0 (next-single-property-change 0 'cmd cmd)))
  (setq cmd (substring-no-properties
             cmd 0 (next-single-property-change 0 'face cmd))))

i.e. fall-back to searching for the change to the face used for in/with to extract the command. However for this to work I should really strip out the 'cmd property on M-i invocations. So this issue is to discuss what the best approach is. I can think of:

  1. custom M-i that replicates ivy-insert-current but with special handling
  2. wrapper M-i that messes with (ivy-state-current ivy-last) somehow - I'm not familiar with how the cl-defstruct stuff works before calling ivy-insert-current
  3. add a hook that ivy-insert-current can process the text with before inserting the string

So @abo-abo which do you think is the best approach? Is there already a mechanism for this sort of thing or is my case too much of a special snowflake because of its heavy use of propertised strings?

@basil-conto
Copy link
Collaborator

I've thought of something like the following:

AKA

(let ((prop (if (get-char-property 0 'cmd cmd) 'cmd 'face)))
  (setq cmd (substring-no-properties
             cmd 0 (next-single-property-change 0 prop cmd))))

@stsquad
Copy link
Contributor Author

stsquad commented Apr 24, 2019

@basil-conto that is tighter thanks. Still need to find the best approach to stripping 'cmd off when editing the selected comment though ;-)

@stsquad
Copy link
Contributor Author

stsquad commented Apr 24, 2019

The other thought I had last night was moving all the in/with stuff to a ivy-format-function so the actual text is just the pure call (although still annotated with blddir/bldenv properties). I got halfway through making those changes before I realised this means the search wouldn't include the path/env values in it which is probably useful.

abo-abo added a commit that referenced this issue May 7, 2019
"M-i" should be equivalent to the user typing out the inserted text,
i.e. without text properties.

Re #2040
@abo-abo
Copy link
Owner

abo-abo commented May 7, 2019

M-i will now insert the text without text properties, since it should simulate the actual input from the user. I don't think it was the intention to pass the text properties before, perhaps an oversight, no code should depend on ivy-text having properties.

Please check if this fixes your issue.

@stsquad
Copy link
Contributor Author

stsquad commented May 7, 2019

Not quite because in the text string I currently include things like the build dir so the line is of the form:

 $(CMD)$(JOIN TEXT)$(BUILD_DIR)$(JOIN TEX)$(ENV)

Where various bits are tagged with different properties. However when executing M-i the user only wants to edit the $(CMD) portion so I'd like to modify the text before it gets inserted into the mini-buffer. Arguably I could move adding all the stuff after $(CMD) to an ivy-format-function but then I'd lose the narrowing of history based on the extra "metadata" of build directory and environment.

@abo-abo
Copy link
Owner

abo-abo commented May 7, 2019

OK, but suppose the user wants to enter the full command manually; M-i is there to facilitate that.

Maybe the commands should not be abbreviated, and instead have the full thing as the candidates.

astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
"M-i" should be equivalent to the user typing out the inserted text,
i.e. without text properties.

Re abo-abo#2040
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

No branches or pull requests

3 participants