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

Move properties into separate package #221

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

TheAssassin
Copy link
Contributor

This cleanup makes working with them more convenient.

Counterpart of t-oster/VisiCut#725.

@mgmax
Copy link
Collaborator

mgmax commented Apr 19, 2024

Looks good.

  • Please resolve conflicts - I can't click the merge button otherwise :-)
  • Can you do some testing to make sure that this does not break loading old VisiCut settings?
    • Suggestion: In the previous VisiCut version, create several laser devices and save some material settings, so that you have XML laserprofile files for each of the property classes that are moved (PowerSpeedFocusFrequencyProperty, ..., whatever). Then look that in the new version you still see the old values.
  • Once this is merged, you can update the PR in VisiCut so that it also includes the new version of the LibLaserCut submodule, and then we can merge that too.

@TheAssassin TheAssassin force-pushed the property-rework branch 2 times, most recently from a6e60e5 to 9567030 Compare April 19, 2024 21:20
@TheAssassin
Copy link
Contributor Author

TheAssassin commented Apr 19, 2024

  • Conflicts resolved
  • Test confirms that the move itself does not break anything

Edit: it's no wonder, given the structure of the XML files, that moving packages does not break anything:

<linked-list>
  <PowerSpeedFocusProperty>
    <power>100</power>
    <speed>80</speed>
    <focus>0.0</focus>
  </PowerSpeedFocusProperty>
</linked-list>

@TheAssassin
Copy link
Contributor Author

I suggest you merge this PR, then I can update a variety of others.

@mgmax
Copy link
Collaborator

mgmax commented Apr 20, 2024

The tests fail because you deleted one Java file. Please try again :)

This cleanup makes working with them more convenient.
@TheAssassin
Copy link
Contributor Author

Somehow, rebasing in this repository is my arch nemesis. Anyway. Let's try again.

@TheAssassin
Copy link
Contributor Author

Here we go.

@mgmax mgmax merged commit 7d20ee0 into t-oster:master Apr 20, 2024
1 check passed
@mgmax
Copy link
Collaborator

mgmax commented Apr 20, 2024

Thanks :-)

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

Successfully merging this pull request may close these issues.

2 participants