pharo
pharo copied to clipboard
pharo-project
Class comment Toggle Edit / View checkbox does not update its state
Bug description In Calypso, the class comment Toggle Edit / View checkbox does not update its state. It reacts on clicks but does not alter its appearance.
I did a PR in the dedicated repository, here:
https://github.com/pillar-markup/BeautifulComments/pull/30
@pavel-krivanek - I did not stumble on this until today and I have seen it in front of my eyes every day. @maxwills While your PR solves the issue outright, the true error is deeper down, in that the isRendering method should work together with the settings stored in BeautifulComments. But alas, it does not. I'll need to look a bit deeper.
While working on the issue I suspected that (because of the old code). I thought that the behavior that was supposed to happen was something more like: There should be a global property (boolean) that determines whether the checkbox is enabled(ie, clickable) or not (which I think it should correspond to BeautifulComments rendering), and a local property (owned by the editor, like isRendering) that determines if the checkbox is checked or not (whose state should be bound to the edit mode of the widget).
However, what I found was that the global setting was used instead of a local property directly to deduce the checked state of the checkbox. And I am unable to see the logic of that (I have no deeper knowledge of the system). That made me doubt of my theory, so I just considered the local property to fix the bug. Why using a local property? Imagine that the user has several editors at once. Clicking one checkbox should not affect the state or behavior of the others in my opinion.
Maybe my confusion is about the purpose of the isRendering field. I thought it was the storage of the boolean bound to the checked state of the checkbox.
In my opinion, isRendering is just the accessor of the local state (I might have misunderstood that), which should be stored and retrieved independently of the global setting. Then it would be other objects' responsibility to decide whether or not allow the edit mode to show based on the global setting and local isRendering.
(I have no intentions of making noise, I'm just trying to help to clarify so we can fix these things in a better way)
It might very well be that all that is really needed is what you propose. I just saw that the setting is broken too, and will need to look deeper at the code.
@maxwills Your analysis is correct. Your PR solves the issue completely.
If there is a problem with the settings is a matter of taste. If one expect the synthesised beautiful microdown to be shown as source when the setting is off, then it works. If one expected only the classic class/package comments, then it the settings are broken. I say leave it as it is until an issue is raised.