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 non-deps and shared UI code in shared folder #9350

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

Conversation

tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Aug 2, 2023

Description

  • Move non-deps module from the deps folder to the shared folder
  • Move UI code that was used in various frontend-plugins as module in the shared folder

Find Qt with versionless removed to avoid this issue on Ubuntu CI: (fixed)

Motivation and Context

Based on ideas from this #9220 (comment)

I would also suggest not adding new things to the "deps" directory - its purpose was for vendors external dependencies, but this PR extends its use for "shared modules". I'd much rather have us use a new directory called "shared", "common", or some other that communicates this purpose.

The solution to this would be to move UI elements shared by modules/plugins to be moved into the same "shared" directory and be made a standalone library

That would also fix this outstanding issue with the frontend plugins which "magically" pull in sources from the UI source tree that they shouldn't actually know about

How Has This Been Tested?

  • Builds
  • PropertiesView are translated correctly.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@tytan652 tytan652 added the Seeking Testers Build artifacts on CI label Aug 2, 2023
@tytan652 tytan652 force-pushed the create_shared_folder branch 10 times, most recently from 3722992 to ebde744 Compare August 5, 2023 22:12
@WizardCM WizardCM added the Code Cleanup Non-breaking change which makes code smaller or more readable label Aug 5, 2023
@tytan652 tytan652 force-pushed the create_shared_folder branch 3 times, most recently from 0de938b to 333e2c0 Compare August 12, 2023 21:28
@tytan652 tytan652 marked this pull request as draft January 13, 2024 09:33
@tytan652
Copy link
Collaborator Author

tytan652 commented Jan 13, 2024

I will push commit by commit to check if each commit (except the a few) builds on Windows and macOS.

@tytan652 tytan652 removed the Seeking Testers Build artifacts on CI label Jan 13, 2024
@tytan652 tytan652 marked this pull request as ready for review January 13, 2024 13:16
@tytan652 tytan652 force-pushed the create_shared_folder branch 2 times, most recently from 2f0c9ec to bbecc61 Compare May 4, 2024 21:42
@tt2468
Copy link
Member

tt2468 commented May 5, 2024

I think common would be a better name for these specific cases. Otherwise, I like this

@tytan652
Copy link
Collaborator Author

tytan652 commented May 5, 2024

I think common would be a better name for these specific cases.

There is this thread #9350 (comment) in the PR, about the name. It was resolved, so I guess it was indeed discussed.

If the name should be (re-)discussed, I would prefer it being done off-thread with a clear consensus on one name.

@PatTheMav
Copy link
Member

I think common would be a better name for these specific cases.

There is this thread #9350 (comment) in the PR, about the name. It was resolved, so I guess it was indeed discussed.

If the name should be (re-)discussed, I would prefer it being done off-thread with a clear consensus on one name.

The result of the discussion boiled down to "we're not entirely happy with 'shared' but also haven't come up with a compelling alternative". Also given that it should be trivial to change this later, we didn't necessarily wanted to block the PR over it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants