Multiline colouration #1888
Multiline colouration #1888
Conversation
|
@Ambrevar Please test this with |
|
Pierre Neidhardt <notifications@github.com> writes:
Don't merge!
I've implemented multiline colouration by re-using the work you did.
I'll keep experimenting and foolproofing the code.
Thanks for working on this.
The changes I made was working quite well but had some problems in some
places, don't remember what though.
In the meantime, could you have a look at my changes?
Yes, ASAP.
…--
Thierry
|
|
@alphapapa: I've tested against |
|
I've updated the changes:
I think the existing code is overly complicated and I'm afraid my patch is not helping. A few suggestions of what we could do:
|
|
@thierryvolpiatto Why putting odd/even properties on singlelines? |
|
Pierre Neidhardt <notifications@github.com> writes:
I've updated the changes:
* The colours have now their own faces.
Why two faces, only one face seems enough
* Movements are working properly it seems.
I think the existing code is overly complicated and I'm afraid my
patch is not helping. A few suggestions of what we could do:
* Remove the separator code if you agree that colour alternation is
superior.
No please keep the original code as it is, also I don't think using a
var to toggle face and separator for multiline is the right thing to do,
using the :multiline slot seems to be a better alternative allowing
sources with separator and others with alternate faces.
E.g. ":multiline 'face" or ":multiline 'separator", ":multiline t"
should be equivalent of ":multiline separator" for backward
compatibility.
…--
Thierry
|
|
Pierre Neidhardt <notifications@github.com> writes:
@thierryvolpiatto
In your original patch, you added this:
do (let ((beg (point)))
(helm-insert-match m 'insert count source)
(put-text-property beg (point) 'helm-candidate
(if (cl-oddp count) 'odd 'even)))
Why putting odd/even properties on singlelines?
Don't remember the details, but I guess we need the property on single
line candidates as well because they should be present among
other multiline candidates.
What we have to do on this issue is first merge the original code
(fix_multiline branch) again into master and see what's wrong with and
without multiline, when this code will be fixed let's apply your patch
on top of this.
Perhaps next week I will be more quiet to look into this more carefully.
Thanks.
…--
Thierry
|
Well, one for odd lines, one for even lines... Did you mean something else? I think we are misunderstood in what we want to achieve: you want sources to define how they display (separator or not) through the slot.
Well, that's what I did: first commit re-applied your changes, second one fixes them (to the extend I've tested) and adds the variables/faces. |
@Ambrevar Because it uses the separators and multi-line candidates in a less-common way. I would like for it to not be broken by this patch. |
The |
|
@alphapapa: Just tested with helm-org-rifle and it seems to work without an itch. |
Duh, I guess you meant we can leave odd entries to the normal background color and only color even entries. Right? I think it's much better indeed. |
|
Pierre Neidhardt <notifications@github.com> writes:
Why two faces, only one face seems enough
Well, one for odd lines, one for even lines... Did you mean something else?
Duh, I guess you meant we can leave odd entries to the normal
background color and only color even entries. Right?
Yes.
I think it's much better indeed. I suggest dark gray and light gray
for dark background and light background respectively.
As you want, take care however to not use something similar to
helm-selection.
…--
Thierry
|
|
Pierre Neidhardt <notifications@github.com> writes:
Don't remember the details, but I guess we need the property on single
line candidates as well because they should be present among
other multiline candidates.
The singleline is not a property of candidates but of the source. A
singleline source will never need colors I believe.
Forget colors for now.
You may have candidates with multilines without having specified the
`:multiline` keyword, e.g.
(helm :sources (helm-build-sync-source "test"
:candidates '("foo\nbar" "baz\nfoo" "bar\nfoo"))
:buffer "*helm test*")
So here a list of 3 candidates which is displayed as a list of 6
candidates in current master branch.
The work I did previously fixed this.
As a matter of fact, the code does not deal with separators, so why
should it deal with colors?
I didn't have the time to investigate much, but I think we should decide
of the semantic for the :multiline keyword and where the colorisation
should happen (not sure yet if helm-render-source is the good place).
I hope I will have more time next week to look into all this more
seriously.
Thanks.
…--
Thierry
|
|
@Ambrevar Thank you for testing it with helm-org-rifle. :) Forgive me if I missed this in the conversation, but please take care to not require applying the alternate colors. For example, helm-org-rifle would not need that, and it would cause problems if it were enabled. |
|
It does not, in fact it works with colors too, albeit useless since helm-org-rifle is using its own separator. As the maintainer of helm-org-rifle, why don't you want to follow upstream? I understand you like empty separators better, but ultimately I believe that choice should be left to the user. |
|
I've removed the colouration of odd multilines. |
I'm not sure why you would think that I don't want to follow upstream. I've worked with Thierry and contributed patches to Helm myself.
When I first created helm-org-rifle, the default Helm multi-line-candidate separator was a plain string of red In the This sounds like a nice change for most Helm commands, but I don't think it would be good to use it in helm-org-rifle, because its results are fontified the same way they appear in Org buffers, and this would interfere with that. So I hope that I won't have to work around this in my own code. |
|
alphapapa <notifications@github.com> writes:
This sounds like a nice change for most Helm commands, but I don't
think it would be good to use it in helm-org-rifle, because its
results are fontified the same way they appear in Org buffers, and
this would interfere with that. So I hope that I won't have to work
around this in my own code.
Nothing will change, I want the default to stay the same as before.
Also there is one new variable `helm-use-multiline-separator` that could be
removed by using only `helm-multiline-separator` with a different value,
like a symbol (face?) instead of a string.
Originally I wanted to use setting by source only using the
:multiline slot, but it looks complicated as we have to handle the
truncated width as well in this slot.
…--
Thierry
|
|
@alphapapa: Sorry for the misunderstanding, I did not mean to undermine your valuable contribution to Helm, far from that! :)
There is no interference: 1. The original code is still here. 2. My patch changes the background colours, thus this won't interfere with any of the usual org fontification. @thierryvolpiatto: What is |
If headers are always starting entries, why not removing the separator altogether? |
That might be helpful in helm-org-rifle, so I look forward to trying that. The hard part may be getting the color right, since it depends on the user's theme. Is there a standard Emacs color for that? I don't think the standard
Because it doesn't look good with them displayed so close together. This is especially so because headers are independent, so they may vary wildly in size, boldness, color, etc, and it looks confusing to have a small, deep heading displayed immediately before a large, shallow heading. Also, entries have entry text, not just heading text, so without a blank line, a line of entry text might be closer to the next heading than to its own heading. Anyway, you can experiment with the code and remove it if you don't like it. |
|
Pierre Neidhardt <notifications@github.com> writes:
@thierryvolpiatto: What is helm-multiline-separator?
Sorry I meant `helm-candidate-separator`.
But I am not really sure on how to enable this for now.
I saw that your patch is working with the alternate faces but breaks
multiline with separator, selection becomes quickly erratic after moving
up and down, wrong candidate is selected when using
helm-move-to-line-cycle-in-source, only first line of some candidates is
selected etc...
…--
Thierry
|
|
I can reproduce two issues:
|
|
I've fixed both issues (first description was inacurate, see the commit). |
|
Pierre Neidhardt <notifications@github.com> writes:
I've fixed both issues (first description was inacurate, see the commit).
Thanks, looks fixed indeed.
However, next/previous-line is now really slow, this is not your additions
that make helm slow but the initial patch (1/4) which need to be
reworked to make helm fast again.
To feel the difference, use master and your branch with M-x
helm-list-elisp-packages-no-fetch and try to move around with C-n/p.
…--
Thierry
|
|
I can't reproduce the performance issue. |
|
Pierre Neidhardt <notifications@github.com> writes:
I can't reproduce the performance issue.
I've tried with emacs-helm.sh and I get the same speed both on master and my branch.
You need more than 5000 candidates and keep pressing <down> or <up> to
really see the difference.
The problem is `next-single-property-change` which is slow.
…--
Thierry
|
|
OK, I can reproduce with 4000 candidates. Let's investigate. |
|
Here is the profile I get when I fire up |
|
Same profiling procedure on master: |
It's enough for odd multilines to use the standard background colour.
This fixes two issues: - With separator, cycling backward from the first entry would skip the last entry. - Without separator, moving upward to an entry without newlines would skip it.
Keep the multiline-related changes of aefc622 while using the control flow of pre-aefc6227.
|
I've fixed the issue. I've rebased my changes over your master branch since there were some conflicts. The fix commit is on my master branch, but somehow GitHub does not see the new commit on this PR. Am I missing something or should I open a new PR? |
|
Pierre Neidhardt <notifications@github.com> writes:
I've fixed the issue.]I've rebased my changes over your master branch
since there were some conflicts. The fix commit is on my master
branch, but somehow GitHub does not see the new commit on this PR.
I can see it from here, I will try and look at your new patch ASAP.
Thanks.
Am I missing something or should I open a new PR?
Perhaps reload the github page ?
…--
Thierry
|
|
Thierry Volpiatto <thievol05@zoho.eu> writes:
Pierre Neidhardt ***@***.***> writes:
> I've fixed the issue.
It is not fixed here it still unusable on large set of candidates.
The problem is coming from `helm--forward-candidate` and
`helm--backward-candidate` that are calling
`next-single-char-property-change` and `previous-single-property-change`
which are really slow.
Sorry to not help more, but I have not much time to read/write code
actually apart for trivial changes, perhaps next week I will have some
time to work on this.
…--
Thierry
|
It took ages to refresh.
I'll see if I can reproduce. |
|
I cannot reproduce: it's really fast with my patch. Here is the profile: |
|
On master, The slow part is seemingly around the new |
|
So the offending part is this: And more specifically: While I understand the last 2 conditions, why the first one? |
|
Pierre Neidhardt <notifications@github.com> writes:
While I understand the last 2 conditions, why the first one?
If this is not multiline, then there should be no separator, or am I mistaken?
helm-pos-multiline-p tells you if you are on a separator line, not you
are using multiline or not.
…--
Thierry
|
|
That's a confusing name then :p Can you detail the meaning of the algorithm? Here is what I understand: Cond:
Assuming first condition is really needed, a possible fix would be to check if it's a multiline source and we are last candidate. But I still don't get the point. |
|
Any insight? |
|
Pierre Neidhardt <notifications@github.com> writes:
Any insight?
I have no idea on how to implement it for now (faster than what we
have).
…--
Thierry
|
|
Can you explain the intention of the first condition mentioned above? Also here is an optimization idea, no clue if that'd be faster though. When looking for the next separator, Instead of scanning all characters, why not scanning the first character of every line? That would probably involve Lisp code instead of C, so that could actually be slower. |
|
Are you dropping this? |
|
Pierre Neidhardt <notifications@github.com> writes:
Are you dropping this?
Yes, this implementation is not the good approach anyway.
Sorry for the long radio-silence, but I was eventually thinking of
giving it another shot at some point. What do you think?
As you want, make another PR if needed, keep in mind what we need is
performance, previous implementation was too slow to be usable.
…--
Thierry
|
Don't merge yet!
I've implemented multiline colouration by re-using the work you did.
I'll keep experimenting and foolproofing the code.
In the meantime, could you have a look at my changes? Please question anything you find dubious.
I am discovering new grounds here, so any guidance is more than welcome :)