search

alphapapa / ement.el

A Matrix client for GNU Emacs
GNU General Public License v3.0
476 stars 44 forks source link

Support RET/M-RET for viewing/scaling images #169

Closed Stebalien closed 1 year ago

Stebalien commented 1 year ago

Refactor ement-room-image-show to ement-room-image-show and ement-room-image-show-mouse. Ditto for -image-scale-. Then bind RET/M-RET to these new commands.

NOTE: this renames ement-room-image-show to ement-room-image-show-mouse for consistency (breaking).

If you'd like to avoid the breaking change, I can rename these functions to -at-point and -mouse, and turn the existing versions into aliases.

alphapapa commented 1 year ago

Thanks, this looks good to me. I think it's okay to rename those commands, since these have always needed improving, and we're far from a 1.0.

Would you mind updating the changelog too? Or I can take care of that if you prefer.

Stebalien commented 1 year ago

Done. In general, would prefer commit amendments (to avoid separate commits addressing reviews) or additional commits (as I did here)?

alphapapa commented 1 year ago

Thanks.

For simple changes like this, I think adding commits makes for easier review; then the branch can squash-merged. For changesets that squash-merging would be inappropriate for, maybe a good protocol would be to make additional commits to address feedback, then rebase with fixup commits and force-push before merging, to make for a cleaner commit history. But it's not easy to give a general rule for when that would be worth the extra effort, and there's always --first-parent for git logs, so...

alphapapa commented 1 year ago

@Stebalien Thank you!