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.

Sign up for GitHub

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

Jump to bottom

[qtpositioning] devendor poly2tri #31462

Merged
merged 2 commits into from
May 22, 2023
Merged

[qtpositioning] devendor poly2tri #31462

merged 2 commits into from
May 22, 2023

Conversation

m-kuhn
Copy link
Contributor

@m-kuhn m-kuhn commented May 16, 2023

Devendor poly2tri from qtpositioning.

@LilyWangLL LilyWangLL self-assigned this May 17, 2023
@LilyWangLL LilyWangLL added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 17, 2023
LilyWangLL
LilyWangLL previously approved these changes May 17, 2023
View reviewed changes
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label May 17, 2023
@dg0yt
Copy link
Contributor

dg0yt commented May 17, 2023

The patch is fairly large simply due to removal of files, and this part will be sensitive to souce changes. I would suggest to trim the patch and to do file removal as needed with cmake commands.

@LilyWangLL LilyWangLL removed the info:reviewed Pull Request changes follow basic guidelines label May 17, 2023
@LilyWangLL
Copy link
Contributor

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@LilyWangLL LilyWangLL marked this pull request as draft May 17, 2023 08:10
@m-kuhn
Copy link
Contributor Author

m-kuhn commented May 17, 2023

The patch is fairly large simply due to removal of files, and this part will be sensitive to souce changes. I would suggest to trim the patch and to do file removal as needed with cmake commands.

Good point. How could this be done?
The download happens in qt_install_submodule in the qtbase port.

  • Patch qt_install_submodule (already done for qttools / qtwebengine)
  • Reimplement qt_install_submodule in qtpositioning
  • Implement some sort of callback mechanism or a SRC_DIRECTORIES_TO_REMOVE param

Neumann-A
Neumann-A reviewed May 17, 2023
View reviewed changes
ports/qtpositioning/devendor-poly2tri.patch
# SPDX-License-Identifier: BSD-3-Clause

# special case begin
-add_subdirectory(3rdparty/poly2tri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this line is not enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for me to be sure during development that there's no reference left somewhere. But effectively, that's the most simple version.

@m-kuhn m-kuhn marked this pull request as ready for review May 17, 2023 10:07
Neumann-A
Neumann-A reviewed May 17, 2023
View reviewed changes
ports/qtpositioning/devendor-poly2tri.patch
Comment on lines +35 to +36
add_subdirectory(3rdparty/clipper)
add_subdirectory(3rdparty/clip2tri)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about those two? Do we need to also devendor those?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @m-kuhn for response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but I don't have an immediate plan for that

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label May 19, 2023
@JavierMatosD JavierMatosD merged commit f8b9c6a into microsoft:master May 22, 2023
@m-kuhn m-kuhn deleted the devendor_poly2tri branch May 22, 2023 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neumann-A Neumann-A Neumann-A left review comments

@LilyWangLL LilyWangLL LilyWangLL approved these changes

Assignees

@LilyWangLL LilyWangLL

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@m-kuhn @dg0yt @LilyWangLL @Neumann-A @JavierMatosD