Ticket #4540 (closed defect: fixed)
Macro in mcedit deletes text because return value of edit_block_delete_cmd is misinterpreted
Reported by: | Sprite_tm | Owned by: | andrew_b |
---|---|---|---|
Priority: | major | Milestone: | 4.8.32 |
Component: | mcedit | Version: | master |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Branch state: | merged | Votes for changeset: | committed-master |
Description
This issue is in 4.8.31 as well as Git master. I'm having issues that macros defined in .local/share/mc/mcedit/macros.d and used as text editing macros using ExecuteScript? in the editor section of mc.macros simply delete any text selected rather than replace the selected text with the output of the macro script.
Seems like commit f29c5a35f7938ab1e7e83a9c9faadc5129e6bb08 reworked the return value of edit_block_delete_cmd from an int with 1 if cancelled by user and 0 otherwise, to a gboolean which is false when cancelled by user and true otherwise. (Note the inverting of logic values in this change.)
Unfortunaty, something in that commit went wrong on https://github.com/MidnightCommander/mc/blob/master/src/editor/edit.c#L1824-L1832 : it looks like this code assumes old 1 is new true and vice versa; this breaks things.
Changing it to 'gboolean rc = TRUE;' and 'if (rc)' fixed this issue for me.
Change History
comment:2 Changed 4 months ago by zaytsev
- Component changed from mc-core to mcedit
- Milestone changed from Future Releases to 4.8.32
Thank you for the report! I think that the declaration is correct, but the check should indeed be inverted as if (rc) :-( yay, tests...
comment:3 Changed 4 months ago by Sprite_tm
Looking at the code, I think I agree, looks like edit_insert_file should not be called at all when there's no mark set. Guess that's a bug in the original code then (it had 'int rc=0' which would translate to a rc=TRUE in the new code.)
comment:4 Changed 4 months ago by andrew_b
- Owner set to andrew_b
- Status changed from new to accepted
- Branch state changed from no branch to on review
Branch: 4540_mcedit_macro_deletes_text
changeset:fdacad8bbb64dc1d6d189dd232008307b77b3642
comment:5 follow-up: ↓ 6 Changed 4 months ago by zaytsev
As I said, I think the default should stay FALSE, that is "it has been canceled by the user" - maybe this is what caused the original confusion. Are you sure that the default has to be changed, maybe I don't understand something about the code? I guess it's not very important though, because the value should in theory be always overwritten if the code is correct, but still...
comment:6 in reply to: ↑ 5 Changed 4 months ago by andrew_b
Replying to zaytsev:
As I said, I think the default should stay FALSE, that is "it has been canceled by the user"
If user canceled his action (closed the user menu), this block of code isn't executed.
If the default value is FALSE and there is no marked block, the text isn't inserted.
How to reproduce:
- gboolean rc = FALSE;
- compile
- run mc -e to open an empty file
- press Shift-F1, select some menu entry (for example, 2 while ()), press Enter.
- result: nothing done
- expected result: inserted the following code:
while() { }
comment:7 Changed 4 months ago by zaytsev
- Votes for changeset set to zaytsev
- Branch state changed from on review to approved
Oh man, of course, you are right. We do not always have a marked block :( Thank you and sorry.
comment:8 Changed 4 months ago by andrew_b
- Status changed from accepted to testing
- Votes for changeset changed from zaytsev to committed-master
- Resolution set to fixed
- Branch state changed from approved to merged
Merged to master: [57cf824a51d98192215f86f4caae3d848da57100].
Whoops, that links the wrong commit and I don't think I can edit it. Proper one is e2e34d82abbc4d674fb965ba087c1a5a6105ea51