-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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.
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
Hide disabled command from context menu #5580
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5580 +/- ##
=======================================
Coverage 86.78% 86.78%
=======================================
Files 594 594
Lines 43017 43042 +25
Branches 7129 7135 +6
=======================================
+ Hits 37331 37355 +24
- Misses 5686 5687 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
src/mouse/touch_handler.js
Outdated
@@ -26,15 +26,16 @@ exports.addTouchListeners = function(el, editor) { | |||
var updateMenu = function() { | |||
var selected = editor.getCopyText(); | |||
var hasUndo = editor.session.getUndoManager().hasUndo(); | |||
var commands = editor.commands.byName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small improvement:
var hasCommand = function(cmd) {
var commands = editor.commands.byName;
if(!commands[cmd]) return false;
if(!editor.getReadOnly()) return false;
return commands[cmd].readOnly;
};
this also disables commands when they are readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to remove non-readonly commands on readonly editor? Or for all editors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whazor did you mean if (!commands[cmd].readOnly && editor.getReadOnly())) return false
for the readonly part?
https://github.com/ajaxorg/ace/blob/master/src/commands/command_manager.js#L48C23-L48C39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it would be nice to hide all commands that are unavailable. So in a readonly editor, only Select/Find/...
After looking at the command manager, maybe we could move the logic from 'exec' to a seperate method (has
?) in the command manager and include both readOnly
and isAvailable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes should make it, I think
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
05149c8
to
a5e52ee
Compare
One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR. |
Issue #, if available: #5579
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Pull Request Checklist:
ace.d.ts
) and its references: