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

Min/Max behaviour broken #9430

Closed
cajus opened this Issue Nov 27, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@cajus
Contributor

cajus commented Nov 27, 2017

#9377 introduces a problem where you can't simply set min/max values to the same. We have to do that in various places and from the naming point of view, nothing tells me that minHeight === maxHeight isn't allowed.

this.set({
  minHeight: value,
  maxHeight: value,
  minWidth: value,
  maxWidth: value
});

Depending on the values that are inside the variables already, it fails. You've to inspect the values first to get out of this. That means that you can't set the value (i.e. in the appearance) for sure. It always depends on what's in there in the time of setting. Relatively impractical.

I didn't understand what PR was for - or why it was needed, so I did not vote for it. So I don't know why the assertion was put there. Its a too early place IMHO. Maybe it should be moved back to the Widget, because it allows the evaluation on render time. Or it should just be a warning and no assertion at all.

@level420

This comment has been minimized.

Show comment
Hide comment
@level420

level420 Nov 28, 2017

Member

@cajus as far as I can see in https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/core/LayoutItem.js#L751-L763
both asserts are about minWidth <= maxWidth and minHeight <= maxHeight where a test against lower or equal is done.
I think this is correct.

Member

level420 commented Nov 28, 2017

@cajus as far as I can see in https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/core/LayoutItem.js#L751-L763
both asserts are about minWidth <= maxWidth and minHeight <= maxHeight where a test against lower or equal is done.
I think this is correct.

@cajus

This comment has been minimized.

Show comment
Hide comment
@cajus

cajus Nov 28, 2017

Contributor

@level420 I didn't say it's wrong. Its just too agressive. Think about a theme A which has a widget with minWidth set to 40 and maxWidth set to 60. Now you switch the theme which has different settings. Because _applyDimensions is now the place where the check happens, it is a hard constraint between these. It was a soft constraint before, because it was checked at render time.

As a result, you now can (by choosing unlucky combinations) not set min/max values after each other in the "wrong" order. You'll directly run into the assertion.

Contributor

cajus commented Nov 28, 2017

@level420 I didn't say it's wrong. Its just too agressive. Think about a theme A which has a widget with minWidth set to 40 and maxWidth set to 60. Now you switch the theme which has different settings. Because _applyDimensions is now the place where the check happens, it is a hard constraint between these. It was a soft constraint before, because it was checked at render time.

As a result, you now can (by choosing unlucky combinations) not set min/max values after each other in the "wrong" order. You'll directly run into the assertion.

@level420

This comment has been minimized.

Show comment
Hide comment
@level420

level420 Nov 28, 2017

Member

@mever could you please have a look into this issue. I'm in favor of reverting that part of the PR which belongs to the min vs. max checks, moving them form qx.ui.core.LayoutItem back to qx.ui.core.Widget. Would this change anything regarding the fix for qx.ui.form.core.VirtualDropDownList?

Member

level420 commented Nov 28, 2017

@mever could you please have a look into this issue. I'm in favor of reverting that part of the PR which belongs to the min vs. max checks, moving them form qx.ui.core.LayoutItem back to qx.ui.core.Widget. Would this change anything regarding the fix for qx.ui.form.core.VirtualDropDownList?

@mever

This comment has been minimized.

Show comment
Hide comment
@mever

mever Nov 28, 2017

Contributor

That's unfortunate. I think that the core of the issue is to set qx-properties in order. We can not guarantee that setting qx-properties in a random order will not break consistency. Therefore I fear reverting will not address the real issue.

To clarify, the set method on qx.core.Object does not respect js-property order (the map argument, which is a js object, is by definition not ordered). The alternative being not able to keep consistent qx-objects while using the set method.

Therefore I think we need to disable constrains during set. And while we're at it we could also make it atomic. Giving it the following properties:

  • Property values are applied atomically. Applying all changes or none, thus one commit.
  • Ordering is solved by running the apply (and event) after all properties are committed (effectively disabling constrains temporary)

The reason why I think we need this is because in my mind an object may contain attributes (thus without integrity/consistency checks) or setters (which can contain logic to maintain consistency). Whereas Qooxdoo is implementing the former as public members and the latter as qx-properties.

Contributor

mever commented Nov 28, 2017

That's unfortunate. I think that the core of the issue is to set qx-properties in order. We can not guarantee that setting qx-properties in a random order will not break consistency. Therefore I fear reverting will not address the real issue.

To clarify, the set method on qx.core.Object does not respect js-property order (the map argument, which is a js object, is by definition not ordered). The alternative being not able to keep consistent qx-objects while using the set method.

Therefore I think we need to disable constrains during set. And while we're at it we could also make it atomic. Giving it the following properties:

  • Property values are applied atomically. Applying all changes or none, thus one commit.
  • Ordering is solved by running the apply (and event) after all properties are committed (effectively disabling constrains temporary)

The reason why I think we need this is because in my mind an object may contain attributes (thus without integrity/consistency checks) or setters (which can contain logic to maintain consistency). Whereas Qooxdoo is implementing the former as public members and the latter as qx-properties.

@mever

This comment has been minimized.

Show comment
Hide comment
@mever

mever Nov 28, 2017

Contributor

@level420 Regarding your question. No it woudn't change anything for VirtualDropDownList.

Contributor

mever commented Nov 28, 2017

@level420 Regarding your question. No it woudn't change anything for VirtualDropDownList.

@mever

This comment has been minimized.

Show comment
Hide comment
@mever

mever Nov 30, 2017

Contributor

I am writing a PR which could solve the problem. This introduces a transaction object. Which will be used by the set method to perform property changes in one transaction (like a database).
This will not only solve the problem with min/max properties specifically but with constraints between properties in general.

Contributor

mever commented Nov 30, 2017

I am writing a PR which could solve the problem. This introduces a transaction object. Which will be used by the set method to perform property changes in one transaction (like a database).
This will not only solve the problem with min/max properties specifically but with constraints between properties in general.

@level420

This comment has been minimized.

Show comment
Hide comment
@level420

level420 Dec 1, 2017

Member

While this is in discussion and the corresponding PR in review, I've created PR #9436 which reverts the assertion.

Member

level420 commented Dec 1, 2017

While this is in discussion and the corresponding PR in review, I've created PR #9436 which reverts the assertion.

@level420

This comment has been minimized.

Show comment
Hide comment
@level420

level420 Dec 20, 2017

Member

Fixed via #9436

Member

level420 commented Dec 20, 2017

Fixed via #9436

@level420 level420 closed this Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment