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 OS' MIDI methods into InputEventMIDI #86713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 2, 2024

Partially addresses godotengine/godot-proposals#8761.
Related to #86716 and #86693.

This PR moves the OS' methods related to MIDI into the InputEventMIDI, as static methods.
The previous methods are still available, but are marked as deprecated.

This means the following:

Currently This PR
OS.open_midi_inputs() InputEventMIDI.open_inputs()
OS.get_connected_midi_inputs() InputEventMIDI.get_connected_inputs()
OS.close_midi_inputs() InputEventMIDI.close_inputs()

The original intention was to move the global constants as well, but that fell through for the time being, and will be done in a separate PR for further discussion.

On the other hand, the methods are comparatively safer to implement and do not break existing compatibility. See the proposal for reasoning.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code this has one issue:
The version in OS is virtual, this means that if this is overridden anywhere the two versions will no longer line up, this needs to be resolved

Currently no system overrides this, but I'd say the method here should call the OS one

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 2, 2024

I updated the PR to have InputEventMIDI's methods call the OS ones as above. The intention for the originals to be deprecated should still be clear.

@Mickeon Mickeon force-pushed the os-to-InputEventMIDI branch from dd15a08 to 87f99ea Compare January 2, 2024 17:25
@Mickeon Mickeon force-pushed the os-to-InputEventMIDI branch from 87f99ea to 9a6da1f Compare January 2, 2024 18:06
@AThousandShips
Copy link
Member

AThousandShips commented Jan 2, 2024

In fact this should probably be handled by updating the regression project, see:
https://github.com/godotengine/regression-test-project/blob/3.x/AutomaticBugs/BasicData.gd#L28

#46183

A solution for this without updating that project would be to match the names of the old OS methods, this would make sure this check doesn't fail, without having to depend on a PR there, and wouldn't be too much of an issue to just have a longer name

@AThousandShips
Copy link
Member

My solution was indeed unfortunately wrong, so the two options now are to either change the name or add to the regression project (and revert my suggestions)

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 2, 2024

Should be easy if it really is just adding "open_inputs" to the exceptions. Although part of me wonders if it's a good opportunity to come up with a better name for these three methods.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 2, 2024

It would be simple yeah but that would have to be merged before this

I would suggest temporarily renaming that method to confirm that the exclusion solves the ci failure

Mickeon added a commit to Mickeon/regression-test-project that referenced this pull request Jan 2, 2024
@Mickeon Mickeon force-pushed the os-to-InputEventMIDI branch from 9a6da1f to 53bae43 Compare January 2, 2024 19:02
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 2, 2024

@Mickeon Mickeon force-pushed the os-to-InputEventMIDI branch from 53bae43 to f645d56 Compare January 2, 2024 20:34
Mickeon added a commit to Mickeon/regression-test-project that referenced this pull request Jan 2, 2024
@Mickeon Mickeon force-pushed the os-to-InputEventMIDI branch from f645d56 to 2b617d7 Compare January 5, 2024 13:30
@AThousandShips AThousandShips added this to the 4.x milestone Jan 12, 2024
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