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

PHStyleContext installing itself into any Morph causes issues #52

Open
LeonMatthes opened this issue Jun 22, 2018 · 2 comments
Open

PHStyleContext installing itself into any Morph causes issues #52

LeonMatthes opened this issue Jun 22, 2018 · 2 comments

Comments

@LeonMatthes
Copy link

We are currently working on our SWT project (PowerSqueak) and we have recently discovered, that a PHStyleContext seems to be created for every morph as an extension, which has been causing all kinds of issues.

First and foremost, when serializing the Morph, its PHStyleContext gets saved with it, which causes severe incompatibility between images when loading .morph files (which basically breaks compatibility of saving and loading in PowerSqueak between users who don't have Pheno installed, and users who have)

This could be fixed, by removing the PHStyleContext before saving a morph, by overriding

Morph>>prepareToBeSaved
    
    ...
    self removeProperty: #styleContext.
    ...

Another issue we ran into occured when we loaded our old SWA game.

For some reason the observers in our Game seem to get an update, with the PHStyleContext as an argument.
We don't know, why this happens, as our game does not even use Pheno in any way, shape or form, its not even built by the toolbuilder. This causes a whole load of MessageNotUnderstood exceptions, as our Observer system does not expect this.

If it is possible, would you please remove the PHStyleContext from any morph that is not currently using pheno

@tom95
Copy link
Owner

tom95 commented Jun 22, 2018

First for the motivation:

Pheno aims to bring a consistent styling to the squeak user interface. It does so by removing constants all over the place. Instead of rewriting the existing Morphic widget, it currently overrides places where magic numbers are used and instead queries the style context. This allows themes to have a great level of control over the appearance of widgets.

Stylecontexts are initalized lazily, as are most extension properties. The fact that virtually all Morphs immediately have one is because they need to setup the hierarchy, by assigning their parents in addedMorph:.

Clearing the stylecontext and thus losing its state in prepareToBeSaved will cause Pheno Widgets to be restored with an inconsistent state. I tentatively included this change and once someone complains (i.e. the usecase is there) we will work on restoring a consistent state after the refstream has been filedin.

Concerning the default observer mechanism: please correct me if I am mistaken here, the way I interpret this system is as a central message bus for any subsystem to inform another. In order for different users to not step on each other's toes, there is the option to set the "aspect" that has changed, so that the observers can decide whether or not to act in response to the change.

Since Morphic is not very specific when it comes to these changes, Pheno has to guess a lot on when it has to request an update, in order to maintain correctness (as verified by the tests). I would welcome PRs to reduce the amount of changed calls or make them more specific, e.g. by replacing them with a custom system that allows for more fine-grained control (e.g. "position in morph changed", "new owner", or just making those aspects that morphic sends) so that Pheno can react more appropriately.

Please tell me if this makes sense to you or if you have a better suggestion, keeping the goals of Pheno in mind. If, in your opinion, changing e.g. your observer implementation is unreasonable, we could also look into splitting the morphic overrides into a separately-loadable metacello group. Again, PR appreciated :)

tom95 added a commit that referenced this issue Jun 22, 2018
…52. WARNING:

this may break saving for your Pheno widgets. If you do notice any odd behavior, please report a bug and we will either revert this change or find the correct fix (which would likely involve reconstructing the consistent state after being restored from a refstream)
@LeonMatthes
Copy link
Author

Well, if its difficult for you to change the update behaviour, I will look into saveguarding our observers.

Regarding the prepareToBeSaved: , I could look into finding out, which Morphs actually modified their styleContext, which would allow the saving of normal Morphs without a styleContext and Morphs explicitly using Pheno with a styleContext (It would not make much sense to store a Pheno Morph without a Pheno attribute just because we are concerned the target image does not have pheno installed, because it would not be able to load the Morph anyway, with a style context, or without).

I will update you if I find a solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants