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

annotations and sorting for activities-completing-read #83

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

jdtsmith
Copy link
Contributor

@jdtsmith jdtsmith commented Apr 20, 2024

(updated) This provides custom annotation and sorting functions for activities-completing-read. It shows the number of buffers (and files), for the last or default state, a magit-style color-coded activity age, and flags for being active or having a modified set of buffers.

This is shown below with vertico but it should support any UI which handles annotation-functions (most do). For theme friendliness, it creates no new faces, instead borrowing the vc-annotate color palette building its own color palette (along with success and warning faces). Happy to take any input/ideas.

image

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Here are a few comments.

activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
@jdtsmith
Copy link
Contributor Author

BTW, are there other pieces of information inside an activity that would be relevant to show that occur to you?

@alphapapa
Copy link
Owner

BTW, are there other pieces of information inside an activity that would be relevant to show that occur to you?

I can't think of any. For myself, the only one that matters is last-used time, anyway. Let's keep this as simple as possible.

@jdtsmith
Copy link
Contributor Author

I could show both default and last ages, but that might be confusing.

Along that line, do you think having the default and last buf+file cnt is useful or TMI? Another approach would be to use some indicator (like *) that the state is "dirty" in the sense that its display buffer list has changed from the default. So buffer cnt + file cnt + dirty flag + last mod age.

@alphapapa
Copy link
Owner

I could show both default and last ages, but that might be confusing.

I don't think that's necessary. This is just for completing-read, not for "management."

Along that line, do you think having the default and last buf+file cnt is useful or TMI? Another approach would be to use some indicator (like *) that the state is "dirty" in the sense that its display buffer list has changed from the default. So buffer cnt + file cnt + dirty flag + last mod age.

For me, it's TMI. I select activities by name, not by a number of how many windows or files are visible in it. I can't think of a situation where I would need to know that.

However, knowing whether an activity has a last-used state could be useful, I think.

@jdtsmith
Copy link
Contributor Author

jdtsmith commented Apr 23, 2024

For me, it's TMI.

I do think I agree. I like having the last buf/file count, since I use multiple "similar" activities, some richer than others (based on screen size). But for the default counts, I'm just comparing to see if they changed at all (i.e. computing a "dirty" flag in my head). For such a flag, what would you recommend? I was thinking "same number of buffers and same buffer names = not dirty".

Update, like this:

image

@alphapapa
Copy link
Owner

I'm not sure what you mean by "dirty." For me what would be interesting is whether the activity has a last-used state or just the default.

@jdtsmith
Copy link
Contributor Author

Sorry, I'm not sure I understand your idea. If you have activity auto-saving on (which is the default), an activity will pretty much always have a "last" state, even if it's only lightly modified from the default (different window points, sizes, etc.). So my idea of "dirty" is if the last state differs is an interesting way from the default — i.e. different buffers showing.

I did need to accommodate a missing "last" state, but that's a pretty rare case for me, like when I've just defined a new state.

@alphapapa
Copy link
Owner

If you have activity auto-saving on (which is the default), an activity will pretty much always have a "last" state, even if it's only lightly modified from the default (different window points, sizes, etc.) ... I did need to accommodate a missing "last" state, but that's a pretty rare case for me, like when I've just defined a new state.

I often use activities-kill, which means that the activity won't have a last-used state.

So my idea of "dirty" is if the last state differs is an interesting way from the default — i.e. different buffers showing.

I feel like that concept might be hard to communicate to the user, but if you want to add an asterisk or something and document it to mean that, I won't object.

@jdtsmith
Copy link
Contributor Author

OK sounds good; lack of asterisk would also mean "no last state".

One oddity I've found: when I revert an activity (which I use instead of kill), I'd expect its last state to be removed. It isn't, and sometimes as a result there are two distinct sets of buffer names for the last and default state, even right after revert runs. This happens if a buffer is a duplicate which has a uniquifyd name. I.e. last might have main.c while default has main.c<proj>. Have you seen this? Perhaps revert should just remove last, like kill does.

@alphapapa
Copy link
Owner

One oddity I've found: when I revert an activity (which I use instead of kill), I'd expect its last state to be removed. It isn't, and sometimes as a result there are two distinct sets of buffer names for the last and default state, even right after revert runs. This happens if a buffer is a duplicate which has a uniquifyd name. I.e. last might have main.c while default has main.c<proj>. Have you seen this? Perhaps revert should just remove last, like kill does.

I can see pros and cons on either side. Please feel free to open a new issue about that if you like.

@jdtsmith
Copy link
Contributor Author

Thanks. I've pushed the * flag and a README update. Let me know if you see anything else that needs addressing.

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Also, could you post a final screenshot showing what it looks like with the asterisk?

activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
@alphapapa
Copy link
Owner

Forgive the additional round of review and comments. If you don't have enough "steam" to make some of the stylistic changes, I can make them myself before merging.

No worries at all. In my view, asking an author to incorporate significant changes, and (far more importantly) support them for years or decades to come calls for real patience. As long as good faith progress is being made towards merge, I'm happy to iterate.

I appreciate that. Sometimes I wonder if I should handle some nitpicks myself before merging; I try to consider whether mentioning it will serve any educational purpose, but then I don't always know how familiar the contributor already is, and whether they just have other preferences.

@jdtsmith
Copy link
Contributor Author

Forgive the additional round of review and comments. If you don't have enough "steam" to make some of the stylistic changes, I can make them myself before merging.

No worries at all. In my view, asking an author to incorporate significant changes, and (far more importantly) support them for years or decades to come calls for real patience. As long as good faith progress is being made towards merge, I'm happy to iterate.

I appreciate that. Sometimes I wonder if I should handle some nitpicks myself before merging;

I'd say feel free to push any changes yourself if they are glaring and not worth describing. But I appreciate the collaborative feel your approach creates. I do a lot of work on other people's packages I admire, and some very capable authors have trouble with that, and can't resist NIH syndrome. So I'd say your approach is very valuable, especially to any less experienced contributors who come along.

I try to consider whether mentioning it will serve any educational purpose, but then I don't always know how familiar the contributor already is, and whether they just have other preferences.

I do have some different preferences but I always enjoy learning how others approach a problem. And I don't mind adapting to the local accent (Canadian ? ;). So some of both here.

As an example I mentioned above, I don't often reach for cl-labels when composing lambda's would do — i.e. when you are just going to pass the function on as an argument, once. But I found to my surprise I like quite the readability and mimetics (of some-func vs. #'my-local-func), so may use that more.

Other than deciding whether we are OK with ½ in the ages I think this is ready from my end. Feel free to season to taste.

@jdtsmith
Copy link
Contributor Author

jdtsmith commented Jul 13, 2024

Actually one more thing: I don't like that oldest-age maximizes across both last and default. It should use last or default, since that's what's shown.

Update: done. This reminds me: where did you learn the pcase .. (cl-struct pattern? I can't find any mention of that in the pcase docs. Quite useful and I learned that here (though I still do also rely on and enjoy the read/write version cl-symbol-macrolet enables, ala with-slots).

These are the ages that are actually showing during completion.
@alphapapa
Copy link
Owner

I do have some different preferences but I always enjoy learning how others approach a problem. And I don't mind adapting to the local accent (Canadian ? ;). So some of both here.

Do I seem Canadian? I'm American, born and raised. :)

C-h f pcase RET shows me:

-- (cl-struct TYPE &rest FIELDS)

Pcase patterns that match cl-struct EXPVAL of type TYPE.
Elements of FIELDS can be of the form (NAME PAT) in which case the
contents of field NAME is matched against PAT, or they can be of
the form NAME which is a shorthand for (NAME NAME).

But that might only show up after loading the relevant library that defines that pcase pattern, IIRC.

Also, FYI:

-- (cl-type TYPE)

Pcase pattern that matches objects of TYPE.
TYPE is a type descriptor as accepted by ‘cl-typep’, which see.

@jdtsmith
Copy link
Contributor Author

C-h f pcase RET shows me:

-- (cl-struct TYPE &rest FIELDS)

Pcase patterns that match cl-struct EXPVAL of type TYPE.
Elements of FIELDS can be of the form (NAME PAT) in which case the
contents of field NAME is matched against PAT, or they can be of
the form NAME which is a shorthand for (NAME NAME).

Ahh, right you are. What's strange is there's nothing in info, my usual go-to for pcase. In general the info and docstring are similar enough that it's strange what is omitted. For example, both include cl-type, but only the docstring mentions seq, radix-tree-leaf and cl-struct.

@alphapapa
Copy link
Owner

Ahh, right you are. What's strange is there's nothing in info, my usual go-to for pcase. In general the info and docstring are similar enough that it's strange what is omitted. For example, both include cl-type, but only the docstring mentions seq, radix-tree-leaf and cl-struct.

Probably an oversight, then, and probably worth a bug report.

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this will be ready to merge after this.

activities.el Outdated Show resolved Hide resolved
activities.el Outdated Show resolved Hide resolved
@alphapapa
Copy link
Owner

Thanks for your considerable patience in this process. One final thing: would you be willing to write a changelog entry for this branch's changes?

@jdtsmith
Copy link
Contributor Author

Thanks for your considerable patience in this process. One final thing: would you be willing to write a changelog entry for this branch's changes?

No problem, done.

@jdtsmith
Copy link
Contributor Author

Anything more to do on this PR, @alphapapa?

@alphapapa
Copy link
Owner

Anything more to do on this PR, @alphapapa?

Thanks for your patience.

By the way, forgive me for asking about this again, but I can't find where I asked you specifically before, and this contribution is larger, and I'm trying to be extra diligent about this when accepting contributions: You have completed the FSF copyright assignment for Emacs, right? I see in emacs.git some contributions from you, but it's not clear whether they are covered by an assignment (e.g. the recent ones under [email protected] say they are exempt).

To be certain, I'm required by the Emacs maintainers to ask them for confirmation for a given name and email address. So is the right name and email address to ask them about "JD Smith <[email protected]>"? (Again, apologies if this is redundant, but I'm trying to be very careful.)

@jdtsmith
Copy link
Contributor Author

I appreciate your diligence (which no doubt saves headaches down the road). Yes, I completed this 23 yrs ago ;). I recently updated my email with the FSF, so that should work for confirmation.

@alphapapa
Copy link
Owner

Thanks! Email sent. Should hear back soon, then I can merge this PR.

@alphapapa
Copy link
Owner

Thanks for your patience. Eli confirmed your CA (a while ago, I just didn't get around to handling the message until now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants