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

[runSofa] Fix compilation when SofaGuiQt is not activated #599

Merged
merged 2 commits into from
Mar 6, 2018

Conversation

fredroy
Copy link
Contributor

@fredroy fredroy commented Feb 26, 2018

It was not possible to compile runSofa without activating SofaGuiQt, because GuiDataRepository was instantiated in SofaGuiQt library.
As GuiDataRepository seems to be rather generic and not tied to Qt, GuiDataRepository has been moved to SofaGuiCommon ; and allows runSofa to be compiled without enabling SOFA_GUI_QT.


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@fredroy fredroy added the pr: fix Fix a bug label Feb 26, 2018
@damienmarchal
Copy link
Contributor

Hi fred,

The sofa::qt::GuiDataRepository is pointing to the set of resources that are specific to the qt application complementing the shared DataRepository ("share/"). Consequently it is the expected behavior to have it only usable when SofaGuiQt is enabled.

I quickly looked in the code base where it is used and the only usage are in qt specific code which shouldn't prevent the compilation. If it does then something is wrong and the right fix is to prevent non qt base code to use this repository.

But maybe the problem you are facing is not a compilation problem but is more about sharing UI resources in different GUI.
To do that I my suggestion is to:

  • always consider to have a specific GuiDataRepository in your application namespace, this repository should point to your application specific resource (the ones that are tied to you GUI functionality).
  • use the DataRepository to store shared UI elements because this one is already shared by sofa (but do we really want to have UI resources in SofaCore ?).
  • or, and I think this is the real way to go, make a plugin (SharedUIResources ) to hold and expose the UI elements we are considering as re-usable by different GUI projects.

But maybe I'm all wrong, if so please tell me.

Damien.

@fredroy
Copy link
Contributor Author

fredroy commented Feb 27, 2018

Hi @damienmarchal

Actually, it was more a linking/DLL problem than a compilation problem. (at least for OS X)
GuiDataRepository is used in runSofa:

GuiDataRepository.addFirstPath(Utils::getSofaPathTo("share/sofa/gui/runSofa/resources").c_str()) ;

runSofa knows the symbol as the header is included ; but if you dont compile SofaGuiQt, the symbol wont be present when the linking occurs (with SofaGuiMain which is supposed to load SofaGuiQt if present)

Anyway, for now I was considering GuiDataRepository as a "global" repository for all GUIs. And making it into SofaGuiCommon seems to do the trick (at least in my point of view). But your solution with the SharedUIResources is more elegant indeed.

@damienmarchal
Copy link
Contributor

damienmarchal commented Feb 27, 2018

Ok I see the problem (why qtcreator is not capable of showing this "usage" of GuiDataRepository is a mystery to me).

Anyhow you are right in moving GuiDataRepository in SofaGuiCommon you need to fix then the path

In SofaGuiCommon.cpp
FileRepository GuiDataRepository("GUI_DATA_PATH", Utils::getSofaPathTo("share/sofa/gui/common/resources").c_str());

And somewhere in qt:
GuiDataRepository.addFirstPath(Utils::getSofaPathTo("share/sofa/gui/qt/resources").c_str()) ;

@fredroy
Copy link
Contributor Author

fredroy commented Feb 27, 2018

Yes, you are right about the path when GuiDataRepository is instanciated !

@fredroy
Copy link
Contributor Author

fredroy commented Feb 27, 2018

[ci-build][with-scene-tests]

@guparan guparan added pr: status to review To notify reviewers to review this pull-request pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Feb 28, 2018
@guparan guparan merged commit 64e2a3d into sofa-framework:master Mar 6, 2018
@guparan guparan added this to the v18.06 milestone Apr 5, 2018
@fredroy fredroy deleted the fix_runsofa_wo_qt branch May 13, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants