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

Moved ControlType and ControlColor objects into class specific namesp… #308

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MartinMueller2003
Copy link
Collaborator

…ace. This resolves a conflict with some other libraries that also use global definitions.

…ace. This resolves a conflict with some other libraries that also use global definitions.
@MartinMueller2003 MartinMueller2003 marked this pull request as draft July 14, 2024 21:13
@MartinMueller2003
Copy link
Collaborator Author

The open questions are:

Is this OK
Should I take it further, Things like the control array are public and they should be made private.

@s00500
Copy link
Owner

s00500 commented Jul 15, 2024

Hm... so this creates a bit of a breaking changes, but I can see the defines are still there ?

I like the idea of addressing the incompatibilities... and I think it is fair enough to go about them like so. I dont think we have any other libraries that have ESPUI as an upstream dependency (not like ArduinoJSON) so I am also less concerned...

As for taking it further, any reason why the control array is public ? I assume nobody uses it in user code right ? Else I would guess it makes sense to go further, any other issues you have in mind here ?

@MartinMueller2003
Copy link
Collaborator Author

I have no idea why it is public. We have accessors to get a control by ID so I do not see a reason for it. The other major breaking change would be to modify the control structure so that its fields are all private. Then force people to use an accessor to modify the values. That will be a significant change to the applications that are taking shortcuts.

As for the #defines, the names are pretty unique and the value is now class specific so the chances for a collision are smaller. If you really want to segregate the code, we could put the ESPUI code into its own name space and then access it with a "using" statement.

Added upgrade instructions.
@MartinMueller2003
Copy link
Collaborator Author

Just added upgrade instructions to the readme and bumped the version to 2.3.1

@s00500
Copy link
Owner

s00500 commented Jul 16, 2024

The ESPUI Verison ? I would rather do that when releasing, not on the merge, but I agree that this should be the next version number.

Shall I release current master as 2.2.4 now to include your upgrades for ArduinoJSON or should we wait till this pr is merged ? (I think a release now makes sense)

@MartinMueller2003
Copy link
Collaborator Author

I would release it as it is. and then we add the breaking change.

@s00500
Copy link
Owner

s00500 commented Jul 16, 2024

did release and updated PIO :-)

@MartinMueller2003
Copy link
Collaborator Author

MartinMueller2003 commented Jul 18, 2024

Update the control management process one level up in OOO. ID and type are now protected by accessors and are now read only.

The management of the list of controls has been gathered into a single file called ESPUIcontrolMgr. This is a singleton factory class responsible for creating, deleting and manipulating controls.

This is about as far as I would want to take things at this time. There are more OOO design patterns that can be applied, but they would be onerous on the existing user base.

@MartinMueller2003 MartinMueller2003 marked this pull request as ready for review July 18, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants