Line dialogs.c:1347 says "/* For a cancel button, use cancel response so user can press escape to cancel */" however in line 1357 only GTK_RESPONSE_CANCEL is linked to response_2, but esc KEY triggers a GTK_RESPONSE_DELETE_EVEN; hence it should be included also in 1357.
I found it because of a strange behaviour when cancelling a keybinding override in preferences dialog. You can reproduce it like so: go to prefs>keybindings, set a keybinding which is already used for another command, then Geany will show up a dialog saying that the combination is already used. Once there, if you press <kbd>Esc</kbd> key, you'll see that it does the "Allow" action, instead of -what the user would expect- the "Cancel" action.
I'm testing it and dialogs work fine as well as the issue exposed above is fixed. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/714
-- Commit Summary --
* Set close dialog and esc key events as cancel in show_prompt()
-- File Changes --
M src/dialogs.c (2)
-- Patch Links --
https://github.com/geany/geany/pull/714.patch https://github.com/geany/geany/pull/714.diff
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714
hello? comments? opinions? anything??
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#issuecomment-154524948
Seems ok to me, @b4n is the GTK-spurt so if he is happy fine.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#issuecomment-156878766
hi! Is there any interest on merging or fixing this??
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#issuecomment-219561098
@b4n ping
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#issuecomment-219570625
Closed #714 via d7750a44796b0ddc1c7a8968fd53aad91382d686.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#event-662369388
The changes seem to achieve the correct results (looking at them, not yet tested), but the function looks already terribly fragile and weird (i.e., what if one of the expected answers was `CANCEL` already?). The odd code here was mapping `"Cancel"` label to `RESPONSE_CANCEL`, and back `RESPONSE_CANCEL` to whatever response was associated with the `"Cancel"` label. Meh. As you say, discarding the dialog emits `RESPONSE_DELETE_EVENT`, not `CANCEL`, so it was effectively useless. Also, knowing the dialog was canceled might be a valuable information to the caller, and it could be handled differently than `response_2` (and why choose `response_2` in the first place? it doesn't have any semantic to it).
Also, it would probably be more solid to `return ret != GTK_RESPONSE_APPLY` instead of `return ret == GTK_RESPONSE_NO` in `kb_find_duplicate()`, so it *allows* only if the response really is `APPLY` and not in any other possible case, no matter what is the response (another bug, future thing in the function, etc.).
I fixed this as I suggest in d7750a44796b0ddc1c7a8968fd53aad91382d686. But thanks for the report, diagnostic and offering a fix!
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/714#issuecomment-219591953
github-comments@lists.geany.org