github-actions[bot]
commented
1 month ago Download the artifacts for this pull request:
Open mahkoh opened 1 month ago
github-actions[bot]
commented
1 month ago Download the artifacts for this pull request:
ruihe774
commented
1 month ago da713ec6300443e6c7d6c317cc0788397a0bfa36 LGTM.
For adf539f1887e340840f6a16fd2cffc3a3b567c9b, I'd rather see a git submodule than see ~7000 lines of addition. This is only my personal opinion.
@Dudemanguy I wonder whether you've been convinced by guys at wayland-scanner#389. I am personally not against their idea.
cc @llyyr @na-na-hi
na-na-hi
commented
1 month ago I think it's better to only include the protocols we want and only update when necessary, and don't use submodule which voids the whole point of vendoring. This is the approach that SDL uses.
ruihe774
commented
1 month ago I think it is acceptable if the curated interface files are less than ~500 lines.
Otherwise, maybe an alternative solution is to put and maintain the interface XML files in a separated repo.
Dudemanguy
commented
1 month ago I don't think the extra maintenance effort is worth it.
mahkoh
commented
1 month ago I think it is acceptable if the curated interface files are less than ~500 lines.
This MR contains only those protocols that are being used by mpv.
kasper93
commented
1 month ago submodule which voids the whole point of vendoring
Why? It is just another way of delivering files, that would otherwise be copied into mpv repository. For me it is unacceptable to ship almost 8k loc of external xml data, that needs to be periodically updated.
dependency('wayland-protocols', version: '>= 1.25', required: get_option('wayland')),
If the problem is with the version, just update the required version to 1.35, it is not our job to distribute those files. And with meson it is as simple as meson wrap install wayland-protocols to allow it to download those xmls if required version is not available in your distro.
mahkoh
commented
1 month ago For me it is unacceptable to ship almost 8k loc of external xml data, that needs to be periodically updated.
There is no need to update the files unless you want to use new functionality added in subsequent protocol versions. And at that point you only have to update the protocol file whose version you want to raise.
kasper93
commented
1 month ago There is no need to update the files
If there was no need to update the files, we wouldn't need this PR or #ifdefs in the code, don't we? Eventually they will be updated, but this is beside the point, I don't think it is good idea to include them, there are clearly better solutions.
mahkoh
commented
1 month ago Eventually they will be updated
Sure but this is just part of normal feature development. If let's say mpv added support for fractional scaling today, then you would have to 1) copy the fractional scaling xml file into the repo and 2) implement the protocol. The burden of copying the file is negligible compared to the actual development work.
There is also little chance of getting it wrong since your code will not compile if you're trying to use a new event or new request from a protocol version that is newer than the checked-in xml file.
I don't think it is good idea to include them, there are clearly better solutions.
Of course. The main goal is to get rid of the conditional compilation.
The interesting stuff happens in the second commit.
The first commit only copies the xml files into the project. Instead, it might be better to use git submodules that can be updated easily. But mpv does not currently use submodules so I didn't want to start doing that.