Reimplement gusbrs's Code for Contribution to GNU ELPA #105
Conversation
The main motivation for this is to replace gusbrs's contribution for inclusion in GNU ELPA. Functional differences: - It no longer touches the user's mark-ring unless they 'stop so the mark is present where corrections began. - Less twidling with marks to get the right result, it is now only used to return to the start when necessary.
There was a problem hiding this comment.
@Provessor thank you very much for doing so much 🙏
Your code looks good. Tests are passing, I've tested it locally and my flows are working.
Regarding mark. Behaviour on master is that whenever you correct a word, it's position is pushed to the mark ring. Some people like it, I don't use it and sometimes I have to do extra pop to get to the place where I wanted to be. As far as I can tell, some users prefer current behaviour, because sometimes you need to do some modifications around the misspelled word. And IMO for such cases there is stop command.
So my take is that I prefer behaviour proposed in your PR + Hydra/Transient interface for those who don't like using flyspell-correct-wrapper with all those universal arguments and commands. Thought the second part is out of context of your change 😄
In addition, I would like to invite @gusbrs to review this PR as well. After all, you are reimplementing his changes and also he was the driver of mark ring improvements 😄
#84 for reference
Perhaps it belongs under a config variable? I'm happy to make such a change in a couple of days (to distance myself from the old implementation again if they're going to have the same featureset) if you want.
|
@d12frosted I'm a little tight at the moment, so I can't really be thorough. But I took a look at the PR and, if I read it correctly, it seems to change current behavior by not setting the mark at all, even when a correction is made (previous lines 424 to 430, which appear not to have an equivalent, and the full restore of the mark ring at the end). Is this understanding correct @Provessor ? (I may be wrong though, please just say it if that's the case). If so, I'd like to argue this is not really a welcome change. Most of my reasoning behind those changes were actually done in #80 , I still think that those considerations are my best shot of how I think things would work best. Particularly, this comment:
From the commit message:
That's what I meant above, so it appears to be intentional.
Well, that depends on a definition of "right result", right? ;-) Anyway, I'm not here to make fuss about it, specially considering the situation. If the change in behavior is intended and @d12frosted agrees with it, it's all fine. I would personally not really celebrate such change, but I'm just one user, and opinions may vary on the subject. The comments above presume an equivalence in functionality would be desirable, and this is something which is not for me to decide. And I'm really just commenting because it was requested. Besides that, the only other comment I would have is not really technical, but formal. Considering the reason for this change, wouldn't it be wiser to literally revert my commit, and then perform any changes after that? I'd think this would make things more robust from a legal standpoint. But, well, the usual disclaimer applies: I'm not a lawyer, etc. |
@gusbrs Yes since making this I've read those commits and related discussions. Now I would like to first make this produce as close of a result as possible then anything else like changing this mark behaviour based on a variable can come later.
Yes I misunderstood the original purpose of changing the mark so much.
That's a good point, I'll see if I know enough about git to make that happen. I've been rather busy the past few days but I would like to have this resolved soon if other work permits. |
|
I've tried to implement the same behaviour after reverting the commits and ended up with almost identical code. I also can't see a clean alternative without touching too much other code. @d12frosted If I have to in order to get different code do you have a preference between also adding features or restructuring the other point/mark management code? |
|
@Provessor apologies for missing your question.
No, as long as @gusbrs is happy about it :) @gusbrs understands mark management in this package better than me |
@d12frosted Sorry, but I would not like to be made an arbiter on this. I was happy to make the contribution originally, and to share an opinion on the PR, as I already did. But I think it is not up to me, and that I offered what I could on this. Whatever the merits of the FSF copyright policy, it does have the side-effect of putting some people aside, me among them. It is not a comfortable position to be in, if one wishes to contribute. It is not your fault, of course, but it is what it is. So, please, do what you think is best here. The track record of |
|
@gusbrs I totally understand this and no need to appologise. And actually it's me who needs to appologise for letting this project being dragged into a land where your contributions are not welcomed. Not that "free as in freedom" after all. Sorry, Gustavo! And I appreciate your contributions 💯 |
|
@d12frosted Well, I have myself concurred it was a good move, when the suggestion was made. I still do. Alas, there are some cons. But it is a trade-off, we should live with it. No reason for you to regret it, or apologize for it. ;-) I just asked for your understanding in letting me "slip away" of the associated chores, which is a fair request in the circumstances. Thank you for that! |
|
Is there any progress on this? :) |
See #99.
I'll leave this as a draft until I'm sure it's good copyright wise.The org commit is 13 lines and one line here is still one line under the limit. I'm happy to make changes if there's some corner case I'm missing or if you don't agree with the changes.I've made it restore the user's original mark-ring (unless they call 'stop). If it was intended as a feature (or if you would prefer it in a separate commit) that the mark was present on the last correction I'm happy to change it back.