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

v84 fix project::addAddon issue #591

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented Nov 11, 2024

baseProject has a virtual class called addAddon(ofAddon & addon)
It was changed to non virtual but it needs to be virtual on the base class, so it is correctly overridden on children classes.

This way base class baseProject::addAddon(const std::string& _addonName) can invoke the specialized function to make the addons work in a specific project

Now VSCodeProject can work correctly again, adding only the selected addons to the file listing in project.

cc: @roymacdonald

@dimitre dimitre merged commit 2e354a7 into openframeworks:master Nov 11, 2024
3 checks passed
@dimitre dimitre deleted the vv84 branch November 11, 2024 23:30
@roymacdonald
Copy link
Contributor

you should not have merged this. it is wrong. it is not fixing anything, but actually making a possible source for future bugs.

Those functions there named addAddon not on baseAddon class are remnmants of the old implementation.
It will actually break the whole thing having it overrinden, it is not the idea.

As I said when this got merged, it is still a work in progress, it still needs cleanup and properly document, so in order to let me do so please dont just go and merge unnecesary stuff.

So, please revert this and I will clean up and comment on how it works. (main.cpp is still a huge mess which needs to be sorted out).

@roymacdonald
Copy link
Contributor

it might work for VSCode what you changed because it mostly relies on make, but it is sort of a cheap hack, but not how it should be done.
The reason for addAddon to be public and not private is because of the huge mess taht main.cpp is, where it is invoked, otherwise it would be private, not even protected.

@dimitre
Copy link
Member Author

dimitre commented Nov 12, 2024

this PR fixes something that was not working.
Let's be friendly and collaborative.

@roymacdonald
Copy link
Contributor

roymacdonald commented Nov 12, 2024

it fixes something but it is a hack. and there are way too many of this sort of hacks in PG, which is what made it such a massive pain to get working properly.
Here is a PR with this same fix but done properly.

TL;DR

There is a lot of logic and parsing in addAddon that needs to be done in order to add get all the projects working. when it was virtual there was a lot of duplicated code in each of the different projects etc. So, the idea is that what gets extended by each project class are these . Then also, only when something different to what is implemented in these functions of baseProject, those should be overriden.

In the particular case of VScode project, I override addAddonBegin, which is there mostly because it was needed in VisualStudioProject to have it happening at the begining of the addAddon function. that is why it is named like that. In the case of VSCodeProject I choose to use it just because it has no clear intention as opposed to something like addAddonFramework, thus serves the purpose. As there is no addAddonToWorkspace or something like that, which feels unnecesary to add and bloats the baseProject class.

In any case, the idea is to explicitly not override addAddon function.

the addAddon functions are still public simply because these are being called from main.cpp, which is a mess I did not got all the way through but I have a good idea of what is going on (there is a lot of duplicated code, and a lot of code that never gets accessed.

as of being collaborative and friendly, I am doing my best considering how frustrating it has been to dive into fix PG and deal with OF's development lately.

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