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

Removing circular dependency with XmlLutReader #1302

Merged
merged 5 commits into from
Jul 1, 2016

Conversation

rinkk
Copy link
Member

@rinkk rinkk commented Jun 29, 2016

Moves Color to DataHolderLib, introduces a new data holder for lookup-tables and removes the circular dependency.

int setColorLookupTable(const std::string &filename)
{ return GeoLib::readColorLookupTable(_colorLookupTable, filename); }
// int setColorLookupTable(const std::string &filename)
// { return GeoLib::readColorLookupTable(_colorLookupTable, filename); }
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to remove from 44-46?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather keep this for now as this is another part of the DE where lookup-tables are used but it would need a complete rewrite. I don't want to make this PR larger than it already is.

@TomFischer
Copy link
Member

Apart from small questions: 👍

#include <QFileDialog>
#include <QSettings>

#include "Applications/DataHolderLib/Color.h"
#include "DetailWindow.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

include this at top

@norihiro-w
Copy link
Collaborator

Changes in this PR are lots compared to just moving XmlLutReader to somewhere under DE (#1284). What is a reason you have to keep it in FileIO? XmlLutReader is used only in DE, isn't it?

@TomFischer
Copy link
Member

TomFischer commented Jun 29, 2016

Changes in this PR are lots compared to just moving XmlLutReader to somewhere under DE (#1284). What is a reason you have to keep it in FileIO? XmlLutReader is used only in DE, isn't it?

One reason is to keep IO functionality local. Another is that it is now possible to use it in other utils without introducing new circular dependencies. External tools like Gocad store specifc color tables which could be reused by paraview or DE.

@norihiro-w
Copy link
Collaborator

Thanks for the two points. However

  • I'm not sure what you mean "local". Moving XmlLutReader under DE actually improves locality because it is only used there.
  • For keeping quality and maintainability of codes, we should keep codes as simple as possible based on current usage. When it comes in the future that someone really needs another tool using color tables, the code should be modified accordingly.

@TomFischer
Copy link
Member

  • By "local" I mean that all IO functionality is grouped together. If you search for a specific interface it should be sufficient to look into the FileIO.
  • I have already a branch for reading Gocad SGrid data and I will use the color lookup table there in the near future.

@norihiro-w
Copy link
Collaborator

I don't agree with the first point (because they are not local from a viewpoint of modularity), but the second point gives some reason to keep it in FileIO. thanks

@rinkk rinkk merged commit 0e70160 into ufz:master Jul 1, 2016
@rinkk rinkk deleted the LookupTableFix branch October 15, 2018 13:02
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

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.

4 participants