Yevgnen / ivy-rich

More friendly interface for ivy.
363 stars 37 forks source link

Discussion about merging the two project? #36

Open casouri opened 6 years ago

casouri commented 6 years ago

I just saw your new branch, and it’s pretty cool! I’d happy to merge my project to yours if we can agree on the code. I haven’t investigate every line but already have some idea based on your README. :smile:

Yevgnen commented 6 years ago

Thanks! We can talk about it tomorrow since my Mac is currently not here right now.

On Sun, Jul 8, 2018 at 04:01 Yuan Fu notifications@github.com wrote:

I just saw your new branch, and it’s pretty cool! I’d happy to merge my project to yours if we can agree on the code. I haven’t investigate every line but already have some idea based on your README. 😄

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Yevgnen/ivy-rich/issues/36, or mute the thread https://github.com/notifications/unsubscribe-auth/AP-U3gO-O7-Cu6rPlqV2D5d_QUig7eknks5uEROSgaJpZM4VGeYG .

casouri commented 6 years ago

Your update is quite complete and I don't think I can add too much to it, except this: (I'm guessing)

ensure that the candidate always have enough space

screen shot 2018-07-08 at 2 53 51 pm

Here you can see that candidate "sssssssssssssssssssssssssssssssssssssssssssss" took space from the part following it.

Also, I configure the length of each part by using a proportion to the max width instead of a fixed length.

Yevgnen commented 6 years ago

Thanks for the details. I'll take a look the implementation details of ivy-filthy-rich later.

The candidate width issue is a bit annoying. In the older version of ivy-rich, long path like /aaaaaaa/bbbbbbb/ccccccc/ddddddd/eeeeeee/fffffff/ggggggg.el that will be shorten into /aaaaaaa/.../ggggggg.el. However, in the customize branch, the max width and the column format function (which only take the candidate as a single argument). I currently have no idea how to make each column always display with reasonable width.

Yevgnen commented 6 years ago

Also, thanks for mentioning ivy-rich in ivy-filthy-rich.

casouri commented 6 years ago

However, in the customize branch, the max width and the column format function (which only take the candidate as a single argument).

What about the max width and format function?

About width issue, to just provide some ideas, in my implementation I use "info-function"s to provide information. For example, ivy-filthy-rich--get-path returns the path of the buffer. However, those functions don't return a string, they return a list of strings, ranging from longest to shortest.

My transformer will then first try to fit the longest string into a column; if it fails, it tries to use the next one. If the last one still doesn't fit, the transformer will simply truncate the string. Note that all this happens before the candidate eats up anyone's place.

I currently have no idea how to make each column always display with reasonable width.

I'm not sure if I understand you, because there is a :width spec in the format, shouldn't that just locks everyone in its place?

Yevgnen commented 6 years ago

Transforming a candidate to a list of candidate string for a transform function seems a bit complicate for users... 😅

I like the idea of ivy-filthy-rich--concat-entry-sequence which allow column overwriting. My original thought is to always shorten/trim every value using ... from the right once they are longer than the max width defined in ivy-rich--display-transformers-list. Maybe we can provide an option to this.

However, this works not so well with file paths since if shortening is necessary, one would usually prefer /aaaaaaa/.../ggggggg.el to /aaaaaaa/bbbbbbb/.... The file path format function does not know whether the length of the candidate is longer than the max allowed value since only the candidate is passed to it as an argument, otherwise the function itself would have to reference to the global value ivy-rich--display-transformers-list explicitly. So it can't perform the above shortening. I'm considering to pass the column properties (:width ... :face ...) as a second argument to the format function as well, but it seems a bit redundant...

casouri commented 6 years ago

How about making The file path format function always return the full path and let the transformer truncate the path on the fly? I'm sure the truncate function knows the required length at that time.

The idea is to separate responsibilities, format functions only provide raw information and the trimming and concatenation are done solely by the transformer.

Yevgnen commented 6 years ago

I agree with that.

Another issue is that, some ivy commands are from swiper, or counsel or even counsel-projectile or other xxx packages. If the minor mode set custom transformers before loading these commands, they may be overwritten when the corresponding package is loaded. Do you have any idea on this? Currently enabling ivy-rich-mode will force to load counsel.

casouri commented 6 years ago

I can think of two ways:

  1. advice ivy-set-display-transformer and filter the transformers. Most packages should follow the rule and use this function. However, if the package modifies ivy--display-transformers-list instead, the advice wouldn't work.

  2. In that case, we can add a function to the specific minor-mode's hook and override the change.

Kaligule commented 3 years ago

Just wanted to mention that invy-filthy-rich is not maintained anymore. So this issue is now basically "try to get as many good ideas as possible from ivy-filthy-rich".