Skip to content
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

Fix problem with initially attached Commands #9275

Merged
merged 3 commits into from Jun 14, 2019

Conversation

@cajus
Copy link
Contributor

commented Jan 24, 2017

This PR fixes three bugs that are Command related:

  • The appearance selector got a wrong value if a Command is attached to a widget during its construction inside the _childControlImpl() method. This is fixed by a temporary $$needsRefresh marker.
  • MExecutable bind the enabled property and keep widgets from beeing disabled when they need to. "enabled" is skipped now. As a result, all Widgets using MExecutable this way ignore the disabled state and were even clickable.
  • MExecutable doesn't care about the enabled state of it's parent widget. As a result it was possible to i.e. trigger a disabled Button with a keyboard shortcut.

This playground example showcases the problem: http://tinyurl.com/hyfojkn Both Buttons should look the same and should be (really) disabled. Not just optically. You can see that the right Button does not get the desired styles and even reacts on clicks/Ctrl+Alt+i when it's disabled.

One problem though: qx has an enabled property on the Command to be able to enable/disable assigned widgets by disabling the related Command. We're loosing this feature with this PR - although I don't think that it's a big loss: it's a designt flaw and mixes scopes.

Closes #5974

@@ -154,7 +154,7 @@ qx.Mixin.define("qx.ui.core.MExecutable",
// remove the old binding
if (old != null && !old.isDisposed() && ids[property] != null)
{
old.removeBinding(ids[property]);
this.removeBinding(ids[property]);

This comment has been minimized.

Copy link
@level420

level420 Jan 24, 2017

Member

Did you use this instead of old because you changed the binding direction here: https://github.com/qooxdoo/qooxdoo/pull/9275/files#diff-3ef8954de08e91faabd2c8f4c7474e0cL180 ?

This comment has been minimized.

Copy link
@level420

level420 Jan 24, 2017

Member

What if old.isDisposed() is true? Is binding removal not needed?

This comment has been minimized.

Copy link
@cajus

cajus Jan 24, 2017

Author Contributor

The original binds from the Command to the Button, i.e. I'm asking myself what the reason for this is, actually. That breaks the enabled state for example. So I changed the direction in the commit. The Command still works and the button still works.

I changed the binding and the unbinding direction. this->value and this->old.

This comment has been minimized.

Copy link
@cajus

cajus Jan 24, 2017

Author Contributor

For the disposal, there might be a problem anyway, because the Button will not be notified when the Command is disposed. You're right in the case of checking. One should change the check above and remove the binding in case of an disposed old.

But that doesn't help if you dispose the Command and don't tell it to the Button.

This comment has been minimized.

Copy link
@level420

level420 Jan 24, 2017

Member

Hmm. travis is failing currently.

This comment has been minimized.

Copy link
@cajus

cajus Jan 24, 2017

Author Contributor

Yes. Because the tests check the opposite direction. They should be updated if this is the way to resolve the problem.

While the changes fix the issues I've here, I'm not sure if it's the right way. It looks like the original qx code is either broken since 1.6 for this (legal) usecase, or I'm not seeing the point.

This comment has been minimized.

Copy link
@level420

level420 Jan 24, 2017

Member

Is the binding direction change needed to solve your problem?

This comment has been minimized.

Copy link
@cajus

cajus Jan 24, 2017

Author Contributor

The $$refreshNeeded solves problem 1. I don't understand why the Command is the master for all these properties. That can't be correct, or am I wrong?

@level420

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

After reading a bit in the source code of qx.ui.command.Command I get the impression that the use case is to have an Instance of qx.ui.command.Command which is added to multiple buttons. See:

https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/command/Command.js#L20-L23

Let's take as an example a command which has a shortcut for paste, e.g. CTRL-v. I think the intention was to have all buttons which are attached to this command be configurable via e.g. command.setEnabled(false).

This way all buttons which are bound to this command get disabled by disabling the command itself. In our example it could be that the paste command is used in a menu button and in a context menu button.

Besides of this explanation, I'm not shure how to solve this. Maybe by manually adding a bind backward from the button to the command, or by adding the command later on to the button after the child control was added to the container.

@level420

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

I think the PR should not change bind direction, as it would break exactly the functionality described in #9275 (comment)

@cajus cajus force-pushed the cajus:fix-command-issues branch from 1b30e5f to b5e822d Jan 25, 2017
@cajus cajus force-pushed the cajus:fix-command-issues branch from a07c1fa to 61b0a70 Jan 25, 2017
@@ -82,7 +82,6 @@ qx.Mixin.define("qx.ui.core.MExecutable",
*/
_bindableProperties :
[
"enabled",

This comment has been minimized.

Copy link
@level420

level420 Jan 26, 2017

Member

But what if a set of buttons which have the same command instance assigned rely on the enable property of the command being forwarded to all those buttons?

This comment has been minimized.

Copy link
@cajus

cajus Jan 26, 2017

Author Contributor

That's disabled. It's a bad idea to do so anyway because of various scopes. You can disable a Command without any problems. But this will not disable the Button.

The way it is designed, does not work: either the widget is ruling the enabled state, or the command. If the command is ruling, it breaks the ordinary disabled state and leaves the button usable, even it looks disabled.

This comment has been minimized.

Copy link
@level420

level420 Jan 26, 2017

Member

Or did I misunderstood the change?

This comment has been minimized.

Copy link
@cajus

cajus Jan 26, 2017

Author Contributor

In other words: the PR makes disabling a Widget and a Command different things. Disabling the Command disables just the Command. Disabling the Buttons, disables the button and takes care it cannot be executed by a Command when beeing in this state.

That's at least not as broken as it is now.

This comment has been minimized.

Copy link
@cajus

cajus Jan 26, 2017

Author Contributor

Btw - what's the usecase of assigning one Command to different Buttons at a time? Doesn't make sense IMHO.

This comment has been minimized.

Copy link
@derrell

derrell Jan 26, 2017

Member

However... disabling a command should not disable all of the widgets to which it is attached. That, I think, was not the intention of Command.

This comment has been minimized.

Copy link
@oetiker

oetiker Feb 3, 2017

Member

I guess the problem depends on who is the 'master' I would assume that the 'widget' is the master ... so if a widget is disabled then there should be no way of activating it ... if a command for activating the widget is disabled, then that should only affect the command and not the widget ...

How about providing commands with a special property so that they will act on disabled widgets all the same to cater for the case @level420 described above.

This comment has been minimized.

Copy link
@johnspackman

johnspackman Feb 3, 2017

Member

Isnt the principal of MExecutable that it's added to a Widget that needs to pull in changes to enabled and other properties from the Command? In which case, it's acceptable for the Widget to bind it's enabled flag or any other property to the Widget it's included into - it's just a design decision.

The problem with enabled is a more general one, IMHO, where different parts of the code want to set enabled and have the values merged in some meaningful way. I have this problem when defining a UI that enables and disables other parts of the UI based on state (eg a CheckBox is off, so therefore a GroupBox of fields is not editable and has enabled set to false); but then there are also user permissions, and if the current user is not allowed to edit any of the fields everything is disabled but the CheckBox is on so the code wants to enable the fields in the GroupBox....

It sounds like @cajus' problem is the same thing, MExecutable's signalling or enabled is conflicting with the enabled setting of other code; but removing it altogether is not the right answer either because there are good reasons for reflecting Command properties in Widget, which is half the reason MExecutable exists in the first place.

EDIT: Perhaps what we really want is a way to merge enabled states through a series of tests or widget hierarchy?

This comment has been minimized.

Copy link
@level420

level420 Feb 3, 2017

Member

Maybe we could make the bindable properties of a command configurable? They are defined in https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/core/MExecutable.js#L83-L92
Something like command.setBindableProperties with defaults as they currently are, not breaking compatibility with current code.

This comment has been minimized.

Copy link
@level420

level420 Feb 3, 2017

Member

BTW: qx.ui.form.CheckBox overrides the _bindableProperties member in order to avoid forwarding of the icon property from the command to the widget. See https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/form/CheckBox.js#L98-L109

@@ -167,6 +168,7 @@ qx.Mixin.define("qx.ui.core.MExecutable",
// check also for themed values [BUG #5906]
if (selfPropertyValue == null) {
// update the appearance to make sure every themed property is up to date
this.$$resyncNeeded = true;

This comment has been minimized.

Copy link
@johnspackman

johnspackman Feb 3, 2017

Member

I don't agree with this - magic member variables that are injected into other classes to invoke special undocumented features should not be allowed. If delayed call to updateAppearance() is necessary for this mixin to work then there's an argument that the same functionality could be necessary elsewhere - after all it is not a requirement that MExecutable is used in something that consumes a Command, it's simply a very helpful mixin.

Ideally the variable in qx.ui.core.Widget should be private or protected and operated via an API.

This comment has been minimized.

Copy link
@cajus

cajus Feb 7, 2017

Author Contributor

I tried various ways. That was the only way to achieve it without changing too much. As it is internal like the other $$ variables, I don't see a real problem there.

This comment has been minimized.

Copy link
@johnspackman

johnspackman Feb 8, 2017

Member

My objection is that $$resyncNeeded is effectively a private state variable for Widget and that by accessing it from outside the Widget class you're breaking a fundamental "blackboxing" principal of OO.

While MExecutable is not really suited to be used except in a class derived from Widget (although it could be), it is perfectly acceptable for a Widget to implement it's own command property should it choose to do so - for example, if the developer decided that they disagreed with how properties are bound from the command, they might decide to avoid MExecutable altogether. This would be reasonable because that's how the code is packaged at the moment. However, now that there is a $$resyncNeeded property, the developer would be forced to use an internal, private state variable to complete the functionality.

Looking at Widget in isolation, $$resyncNeeded is largely undocumented, and it's increasingly confusing as to what's going on when there is also .syncAppearance() (which is called immediately after MExecutable sets $$resyncNeeded=true) and also .updateAppearance().

I guess my point is that not having deeply looked at this piece of code before, it's apparent that there are some significant and sometime subtle things going on in Widget and IMHO that makes it especially important that behaviours of the Widget are very clearly defined and make sense all on their own.

Copy link
Member

left a comment

With this change, are these conditions met?

  • the bug, if we agree to call it that, that a disabled widget can be affected (other than its enabled state) by a Command is fixed.
  • a single Command can be attached to multiple widgets.
  • it is still possible to create a command that alters the enabled state of all of its attached buttons

If not, I believe we have backward incompatibility issues to address, aside from deciding if the changes are appropriate.

As additional explanation to my prior comments, I do NOT think it's appropriate for the command.setEnabled() to change the enabled state of the command's attached buttons -- command.setEnabled() should only affect the Command itself -- but it should be possible for the Command's listener to change the state of its attached buttons.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

AIUI points 2 & 3 are "yes" (ie unchanged), but the answer to point 1 is that the widget is still updated, it's just that the enabled property is excluded from the list of properties which are changed.

The Command isn't pushing changes into the button, the button is pulling them in by choice, because it chose to include MExecutable, and because the user application chose to call .setCommand(...).

IMHO that's the point of at least 50% of the code in MExecutable, to copy property values from the Command to the Widget - the rest of the code is about copying the "execute" event from the button, so you could say that copying Command to a button is the entire purpose of MExecutable.

The properties which are copied are almost all about the UI - label, icon, tooltipText, and menu, plus value - tie the behaviour of the Command to the buttons pretty closely anyway, and enabled is just another one of those UI properties.

MExecutable already provides a mechanism for the list of properties being copied to be altered on a per-button basis because _bindableProperties can be modified to suit, or the user application can avoid it altogether by simply not calling button.setCommand(...) and replace it with it's own bindings.

@derrell

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

I think we may be talking around each other. Am I correct that you are proposing that it is ok, for example, for a Command to execute a button's listener even though the button is disabled?

If I understand the purpose of this PR, it is to "fix" that: to make it impossible for a Command do a button's action if that button is disabled.

I'm just trying to understand the arguments here...

@johnspackman

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

no, not at all - that should be fixed by checking the enabled flag before firing the event, it's just that making a change to not copy the enabled property is a breaking change, and Im suggesting that it's valid to copy the enabled property just like all the other properties like label etc.

@level420

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

Just to sum up my proposal discussed on gitter.im/qooxdoo/qooxdoo:

I'd vote for an alternative solution, where the command could modify the bindable properties list during instantiation as an optional, second parameter for the constructor, which preserves the current behavior. This way you could individually decide to exclude the enabled property from the properties to be bound. If not present, the default list of properties, from qx.ui.core.MExecutable should be used, which is defined here:

https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/core/MExecutable.js#L83-L92

Calling execute of a disabled widget should be prevented, by adding

  if(this.getEnabled()===false) {
    return;
  }

here: https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/core/MExecutable.js#L99

I'm not sure if it solves all aspects of his PR, but it would solve the need to not have the enabled property of the widget being bound to the command
This would be:

var command = new qx.ui.command.Command("Ctrl+Alt+v",  [
      // "enabled",
      "label",
      "icon",
      "toolTipText",
      "value",
      "menu"
    ]
);

where the enabled property is not included in the bind-able properties, or you could completely omit binding via

var command = new qx.ui.command.Command("Ctrl+Alt+v", []);
@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

@level420 I don't think that it is intuitive or obvious why one has to list bindable properties. It's just a try to keep the broken stuff going. I'd just remove the "enabled" from the list of bindings as proposed and document it in the changes. I suspect that too many people are using Command recycling.

@level420

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

It could also be implemented as a "do NOT bind those properties" array. So having

var command = new qx.ui.command.Command("Ctrl+Alt+v",  [ "enabled" ]);

would then exclude the enabled property from being bound.

@oetiker

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

@cajus is the description at the start of the PR in sync which what it does now?

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

@oetiker yes. "enable" is removed from the bindings and a re-trigger flag is added. Maybe it is possible to use a private class member as @johnspackman suggested, but then we've to add some additional flags to the updateAppearance() calls.

@level420

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

@cajus just a short question: when "enable" is removed from the bindings: is then the $$needRefresh appearance refresh part still needed?

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

@level420 yes, these are two different issues.

@level420

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

@cajus what if there are no bindings between the command and the widget. Would then the issue still be there?

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

@level420 if we completely skip all bindings, it works, too.

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

Ping. From my feeling I answered all questions regarding this PR. Any ideas whether we just close it or if I can do anything to increase the acceptance?

@johnspackman

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

Re-reading all of this I think that the issues raised revolve around how Widget.enabled works (including how I'd like it to work 😉 ) and are beyond the scope of this PR; fundamentally, the issues listed at the top of the PR are significant issues and should be resolved.

This PR solves the issues so I'm withdrawing my objection and approving.

Copy link
Member

left a comment

I agree with @johnspackman ship it

@level420

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

@cajus: As I remember it would stop disabling widgets if the command is disabled, as the enabled property isn't bound anymore, right? This is breaking backwards compatibility.
I'm still not fully convinced by this PR and made some proposals on keeping the backwards compatibility which are not really good as I have to admit.
I'll stay neutral for this PR. Let's wait what @derrel says.

@derrell

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

I'm not sure that my questions in my Feb 3 comment, repeated here, have been answered, so I'm still not sure of my opinion on the PR:

With this change, are these conditions met?

  • the bug, if we agree to call it that, that a disabled widget can be affected (other than its enabled state) by a Command is fixed.
  • a single Command can be attached to multiple widgets.
  • it is still possible to create a command that alters the enabled state of all of its attached buttons

If not, I believe we have backward incompatibility issues to address, aside from deciding if the changes are appropriate.

As additional explanation to my prior comments, I do NOT think it's appropriate for the command.setEnabled() to change the enabled state of the command's attached buttons -- command.setEnabled() should only affect the Command itself -- but it should be possible for the Command's listener to change the state of its attached buttons.

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

@derrell the complete Command thingie is a big misconception, because you have multiple sources for the enabled flag. I don't think that it's possible to change it without breaking the previous behaviour. I even don't think that your 3rd point is valid at all. Or at least I can't see the usecase. There's nothing changed for your 2nd point. I don't understand the first point, though.

Everything else is in the updated issue description. It fixes three bugs, and - if no unique decision is possible - maybe we should just fix the appearance queue bug. And document the other annoying "feature" (no way to really disable buttons with commands attached. I'll keep it in the list of patches that I've to apply for our projects. No problem with that. Or at least better than using to much resources in writing about broken button behaviour ;-)

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

Or what about this approach: lets define how we think that the behaviour would be correct. I'll adapt the PR accordingly. Based on the Playground example:

  • Widgets with a Command tend to not respect their appearance. That is a wrong behaviour, because we want these widgets look like we want to. Guess that's something we could agree on.
  • Widgets with a Command can be disabled programmatically, look disabled but are clickable and reachable for keyboard shortcuts. This is also something which sounds wrong and we could agree on.

The first can be fixed by a patch that doesn't change any behaviour. The second - well. Maybe we can achieve this behaviour:

  • Widgets with Commands that are disabled (from within the widget) are disabled for whatever interaction and look disabled
  • Widgets with Commands that are disabled (from within the Command) are disabled for whatever interaction and look disabled

That would involve some more modification in some places, but maybe @level420 and @derrell are more happy with it? If not, what is your expected behaviour in this case?

@derrell

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

I am liking this approach, as I think I will understand it better.

Firstly, attaching a command to a widget should have absolutely no, zero, zilch effect on its appearance. A command adds keyboard control and potentially an action to a control, but has nothing to do with what it looks like. If the fact that there's a command attached to the widget is what causes the widget to look different, that's a bug that needs fixing. That one's easy.

If a widget is disabled, there is nothing that the mouse or keyboard should be able to do to cause the widget's actions. An attached Command, acting solely as an attached command, has no special privileges in this regard. Also easy.

That said, a Command can have an "execute" listener. Disabling the widget to which a Command is attached does not (and should not) disable the Command itself, so adding a listener on a Command's "execute" event, that happens to have code that enables a disabled widget (whether or not it's a widget to which the Command is attached) is perfectly acceptable.

And to close the loop, the Command itself can also be disabled, in which case its actions on a widget to which it is attached stop happening, as do firing of its events. But disabling the Command does not by itself disable the widget; the two are independent.

That's my feeling about how this stuff should work. Thoughts?

@level420

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

OK. I'm nearly convinced to accept @cajus ' PR and am following @derrels last comment. What I'm worried about is if we are breaking something or loosing functionality we had before, as qx.ui.core.MExecutable is used in 9 framework classes (see https://github.com/qooxdoo/qooxdoo/search?utf8=%E2%9C%93&q=qx.ui.core.MExecutable ), amongst them qx.ui.form.CheckBox, qx.ui.form.RadioButton and qx.ui.form.Button. Ommitting binding the enabled properties in could have effects here which are not desireable.
@cajus Are there any negative effects you found regarding those classes or better widgets?

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

I guess the PR does what @derrell suggests. I've basically removed the binding of "enabled". So Command and Button go their own way for "enabled". If the buttons execute is used, it checks whether it's enabled or not. If the commands execute is used, it does not check. So you've to disable the command, too.

@cajus

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

But you're right - this is definitely a different behaviour like it was before.

@pullapprove pullapprove bot requested review from cboulanger, level420 and zaucker Jun 12, 2019
@cboulanger cboulanger requested a review from hkollmann as a code owner Jun 14, 2019
@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@derrell Can you revisit this issue and make a decision? Thank you.

@cboulanger cboulanger removed the request for review from zaucker Jun 14, 2019
@hkollmann hkollmann added the blocker label Jun 14, 2019
@hkollmann hkollmann merged commit e66a073 into qooxdoo:master Jun 14, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0004%) to 55.512%
Details
pullapprove 1 group approved
Details
johnspackman added a commit to johnspackman/qooxdoo that referenced this pull request Jun 15, 2019
* commit 'e66a073fc00b9ae8417ddbbba49b6cdfeba96208': (63 commits)
  Fix problem with initially attached Commands (qooxdoo#9275)
  Add ability to return a qx.Promise (qooxdoo#9682)
  Update .travis.yml
  Update .travis.yml
  Update .pullapprove.yml
  Remove live localhost link
  Test for a documentation PR (qooxdoo#9694)
  Updating readme
  use always the newest qooxdoo compiler to compile framework (qooxdoo#9695)
  Update .travis.yml
  Update check-if-docs-only
  Update .travis.yml
  Lint markdown files and skip tests if only markdown files have been committed  (qooxdoo#9697)
  Update .pullapprove.yml
  Update as per @derrell's change request
  This adds an emergy hotfix rule to the pullapprove rules
  fix ignores for @qooxdoo/compiler (qooxdoo#9677)
  Add a pullapprove.com config file (qooxdoo#9689)
  urgent: fix missing type in compile.json. Default is "web", with this the npm package will not work! (qooxdoo#9690)
  Fix deploy to npm (qooxdoo#9679)
  ...

# Conflicts:
#	index.html
lucapivato added a commit to iceteagroup/qooxdoo that referenced this pull request Jun 28, 2019
ronkie added a commit to RocketSoftware/qooxdoo that referenced this pull request Jul 9, 2019
johnspackman added a commit to johnspackman/qooxdoo that referenced this pull request Aug 14, 2019
* commit '2914d890daae46bafa4338fc35aa1a29d74b963b': (77 commits)
  add demo apps to npm build (qooxdoo#9712)
  Use locally cached docsify assets (qooxdoo#9710)
  Abort on cancel (qooxdoo#9704)
  qooxdoo npm source package should live under source like all other packages. (qooxdoo#9707)
  let index.html show to qooxdoo repos (qooxdoo#9703)
  add CapsLock detection (qooxdoo#9706)
  [travis-docs-only] adds documentation for qooxdoo/qooxdoo-compiler#448 (qooxdoo#9701)
  Add the documentation folder to the npm package (qooxdoo#9702)
  Adapt to new url, fix some image paths (qooxdoo#9700)
  Fix problem with initially attached Commands (qooxdoo#9275)
  Add ability to return a qx.Promise (qooxdoo#9682)
  Update .travis.yml
  Update .travis.yml
  Update .pullapprove.yml
  Remove live localhost link
  Test for a documentation PR (qooxdoo#9694)
  Updating readme
  use always the newest qooxdoo compiler to compile framework (qooxdoo#9695)
  Update .travis.yml
  Update check-if-docs-only
  ...

# Conflicts:
#	index.html
@rad-pat

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Is there some way this can be made backwards-compatible? Buttons are now enabled throughout our application because this behaviour had been relied upon. I need a quick fix rather than having to trawl all modules fixing up the buttons/menus - I'm happy to do that later.
Perhaps conditionally defining the _bindableproperties such as: _bindableProperties : qx.core.Environment.select("qx.legacycommandbehaviour", { "true": [ "enabled", "label", "icon", "toolTipText", "value", "menu" ], "false": [ "label", "icon", "toolTipText", "value", "menu" ]}),

@johnspackman

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@rad-pat sounds reasonable to me; please create a PR

@derrell

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@rad-pat Just so we fully understand what's happening in your case, are you saying that in your code, you depend on command.setEnabled(false); to also disable the widget to which the command is attached? If that's not the issue, what behavior were you depending on that became unavailable with this change?

@rad-pat

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Yes @derrell, sorry if I wasn't clear. It's exactly as you stated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.