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.

Sign up for GitHub

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

Jump to bottom

Allow user to customise reaction function #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow user to customise reaction function #37

wants to merge 1 commit into from

Conversation

9viz
Copy link
Contributor

@9viz 9viz commented Sep 20, 2021

As discussed, the reaction prompt function is now customisable. Hopefully, I got the conventions right.

@alphapapa
Copy link
Owner

As discussed in the chat room, please try to get the FSF copyright assignment done. If you're unable to, I'll rewrite this (very minor) patch myself. Thanks.

@alphapapa alphapapa added enhancement New feature or request waiting labels Sep 24, 2021
@alphapapa alphapapa self-assigned this Sep 24, 2021
@9viz
Copy link
Contributor Author

9viz commented Mar 5, 2022

Update: I mailed the assign@gnu.org for the FSF CA for Emacs (not GNU ELPA) last month, and I am waiting for their reply now. I will update once again when they get back to me.

@alphapapa alphapapa force-pushed the master branch 2 times, most recently from ea29504 to 2c5bf38 Compare April 9, 2022 22:20
@alphapapa alphapapa force-pushed the master branch 2 times, most recently from 12c2abf to 2e072ef Compare April 15, 2022 15:30
@9viz
Copy link
Contributor Author

9viz commented Apr 20, 2022 •
edited
Loading

Finally got it done. I finished the copyright assignment for Emacs.

@9viz
Copy link
Contributor Author

9viz commented Apr 22, 2022 •
edited
Loading

@alphapapa Please check if everything is okay!

P.S. I merged manually so I might have messed something up.

alphapapa
alphapapa requested changes Apr 22, 2022
View reviewed changes
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 left a few comments. Please also rebase the patch on current master so it can just be applied as a fast-forward. :)

ement-room.el Outdated Show resolved Hide resolved
ement-room.el Outdated Show resolved Hide resolved
ement-room.el Outdated Show resolved Hide resolved
@9viz 9viz force-pushed the scratch/custom-reaction-function branch from 89a2c7f to 579b817 Compare April 22, 2022 17:10
@9viz
Copy link
Contributor Author

9viz commented Apr 22, 2022

Ugh, those pesky indentation changes made it in! I forgot to check the diff before committing :(

alphapapa
alphapapa requested changes Apr 22, 2022
View reviewed changes
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 left one more change request comment. Also:

Ugh, those pesky indentation changes made it in! I forgot to check the diff before committing :(

Yes, please remove any whitespace-only hunks from the commit (Magit makes it easy to kill/revert individual hunks).

ement.el Outdated Show resolved Hide resolved
@9viz
Copy link
Contributor Author

9viz commented Apr 23, 2022

Now done. I also reverted the whitespace only chanegs that sneaked in last time (this probably clutters the git history tho :/).

@alphapapa
Copy link
Owner

alphapapa commented Apr 23, 2022 via email

@9viz 9viz force-pushed the scratch/custom-reaction-function branch from 9b473a2 to 289979c Compare April 23, 2022 05:57
@9viz
Copy link
Contributor Author

9viz commented Apr 23, 2022

Now done

@alphapapa
Copy link
Owner

Thanks. Unfortunately this branch seems to have unbalanced parens. Please lint the file you changed. As well, I'll mention a couple of final nitpicks as comments...

alphapapa
alphapapa requested changes Apr 29, 2022
View reviewed changes
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.

ement.el Outdated
"Function that prompts the user for reaction string.
The function is called with no arguments and should return a
string to be used as the reaction."
:type '(choice
Copy link
Owner

Choose a reason for hiding this comment

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

The following lists are short enough that this line break can be removed. :)

Copy link
Owner

Choose a reason for hiding this comment

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

This still needs to be fixed.

ement.el Outdated
The function is called with no arguments and should return a
string to be used as the reaction."
:type '(choice
(const :tag "React character" #'ement-read-reaction)
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the tag "Default function" for this.

Copy link
Owner

Choose a reason for hiding this comment

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

This too.

@9viz 9viz force-pushed the scratch/custom-reaction-function branch from 289979c to d0b163a Compare April 30, 2022 03:46
@9viz
Copy link
Contributor Author

9viz commented Apr 30, 2022

Thanks. Unfortunately this branch seems to have unbalanced parens. Please lint the file you changed. As well, I'll mention a couple of final nitpicks as comments...

Looks like blindly trusting smerge turned out to be a mistake? Anyway, this should be fixed now, I hope.

alphapapa
alphapapa requested changes May 2, 2022
View reviewed changes
ement.el Outdated
"Function that prompts the user for reaction string.
The function is called with no arguments and should return a
string to be used as the reaction."
:type '(choice
Copy link
Owner

Choose a reason for hiding this comment

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

This still needs to be fixed.

ement.el Outdated
The function is called with no arguments and should return a
string to be used as the reaction."
:type '(choice
(const :tag "React character" #'ement-read-reaction)
Copy link
Owner

Choose a reason for hiding this comment

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

This too.

@9viz 9viz force-pushed the scratch/custom-reaction-function branch from d0b163a to 43648ca Compare May 2, 2022 16:06
@9viz
Copy link
Contributor Author

9viz commented May 2, 2022

Ah, my bad, I forgot to push the changes!

@alphapapa
Copy link
Owner

Thanks. It's almost ready to merge, but there are two small issues that need to be fixed, which I found using makem.sh lint:

In ement-room-send-reaction:
ement-room.el:1529:20: Warning: reference to free variable
    `ement-read-reaction'
ERROR (2022-05-05 08:36:06): Linting compilation failed.
ement-room.el:1530: Indentation mismatch
ement-room.el:1530: Indentation mismatch
ERROR (2022-05-05 08:36:08): Linting indentation failed.

Since ement-read-reaction is defined in another file, you'll need to add a (defvar ement-read-reaction) before that variable is used. Look in ement-room.el for ;; Variables from other files.. Then if you'll fix the indentation of those lines (you could just remove the linebreak after (list), this can be merged.

@9viz 9viz force-pushed the scratch/custom-reaction-function branch from 43648ca to bf707b0 Compare May 8, 2022 16:37
@9viz 9viz force-pushed the scratch/custom-reaction-function branch from bf707b0 to 31c52d9 Compare May 8, 2022 16:41
@9viz
Copy link
Contributor Author

9viz commented May 8, 2022

I hope I got the line number right. 1530 led me somewhere else (I did not see a list form there at least). I cannot run makem.sh on my end and I don't really understand how to make it work either (I tried running the indentation checker function in my Emacsen and it reported pretty much every line?).

@alphapapa
Copy link
Owner

Here's how I use makem.sh on this project: M-x compile RET, replacing the command with ./makem.sh -s.sandbox --install-deps --install-linters lint.

@alphapapa alphapapa force-pushed the master branch 3 times, most recently from 4a42004 to 481f571 Compare July 21, 2022 15:38
@alphapapa
Copy link
Owner

@Vizs FYI, now you can use this Transient menu to run makem: https://github.com/alphapapa/makem.sh#transient-menu-makemel

@9viz
Copy link
Contributor Author

9viz commented Sep 9, 2022 via email

@alphapapa alphapapa force-pushed the master branch 2 times, most recently from 7d2e0f2 to c0b922b Compare May 14, 2023 04:24
@alphapapa alphapapa added this to the Future milestone Jul 9, 2023
@phil-s
Copy link

phil-s commented Apr 18, 2024

IIUC the new ement-room-reaction-picker covers at least part of this functionality (and maybe all of it). Off the top of my head I'm not sure if the specific ement-read-reaction behaviour is provided by the options in 0.15, but I think there's probably one which is sufficiently similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alphapapa alphapapa alphapapa requested changes

Assignees

@alphapapa alphapapa

Labels
enhancement New feature or request waiting
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@9viz @alphapapa @phil-s