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

remove root #15

Open
tlmquintino opened this issue Aug 10, 2011 · 19 comments
Open

remove root #15

tlmquintino opened this issue Aug 10, 2011 · 19 comments
Assignees
Labels

Comments

@tlmquintino
Copy link
Member

we should rename root from the current //Root to simply "/"

@ghost ghost assigned gquentin Aug 10, 2011
@tlmquintino
Copy link
Member Author

change URI scheme separator to ://

@barche
Copy link
Member

barche commented Sep 9, 2011

This has too much impact to be possible to complete before the release. We should also think this through carefully:

  • is a global TOC (stored at root) necessary?
  • In theory, storing parent, name and children at each component is enough for it to find its path, no need to store paths explicitely?
  • the "free component" problem would be solved implicitly by not allowing path operations on components that have no parent

@wdeconinck
Copy link
Member

This is not about the TOC, which is surely not coming this release (or after).
This is only about renaming the root, allowing for more standardized paths.
cpath:///Model/Domain/Mesh
instead of
cpath://Root/Model/Domain/Mesh
similar to file:///Users/willem/Documents

The problem is that renaming the root to be a slash is an illegal name at the moment for a component.
This should not be too difficult to change.

On the side:
It is safe to assume there is only 1 root.
Maybe therefore the root should become a singleton. Some tests will need to be adapted that create multiple roots.

@tlmquintino
Copy link
Member Author

IMHO: the root should not become a singleton.

I think we should follow the rule to avoid singletons as much as possible.

For example, we were planning to have multiple so called 'workspaces' to allow the existence of multiple connected simulations. This - could - be done with multiple roots (although I dont defend any design right now).

In practice our root is only one, because we access it via the Core::instance().root() - and Core is a singleton.

@gquentin
Copy link
Member

gquentin commented Sep 9, 2011

I agree that the root cannot be a singleton. The client manages two roots : one from its own Core instance and another one for the mirrored tree. They are separated to avoid conflicts with the subcomponents such as "Environment", "Libraries", "Tools",...

Currently, the root path is "/" and I think it should remain like that (like when you do pwd from the filesystem root, you get '/'). Then we should either give an empty name to the root (illegal for the moment) or give a name that is ignored when build the component paths.

@tlmquintino
Copy link
Member Author

I agree with Quentin.
Let is be an empty name that gets especially treated in the function that checks name validity.

@ghost ghost assigned barche Oct 25, 2011
@barche
Copy link
Member

barche commented Oct 25, 2011

OK, I slightly changed the title of this issue (used to be rename root).

With the changes related to issue #48 and #107, the component lookup is basically such that root becomes "implicit", i.e. a parentless component is the head of the tree, and would always be / regardless of its name (so the name of root doesn't appear in any path, but it can be named whatever you want). In the current implementation, root is still used, but it would be very easy to remove now and replace with a CGroup.

@tlmquintino
Copy link
Member Author

Nice! This makes things more simple and clear.
However, we need clarify in the documentation that

cpath://hostname:port/path/to/component

accesses then the component starting on root Core::instance().root().

@barche
Copy link
Member

barche commented Oct 26, 2011

Root can be removed after the notification queue system is merged into EventHandler. Need to discuss with @gquentin

@gquentin
Copy link
Member

OK to remove the root.

For the removing of the queue, this should no be hard. Do you want me to do it, or you prefer to do it yourself?

@barche
Copy link
Member

barche commented Oct 27, 2011

If you have the time, I'd prefer that you do it, since it's not clear to me how it works, exactly.

@gquentin
Copy link
Member

OK, I'll that today.

barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
gquentin added a commit to gquentin/coolfluid3 that referenced this issue Oct 28, 2011
gquentin added a commit that referenced this issue Oct 28, 2011
Get rid of event management through the root (issue #15)
@gquentin
Copy link
Member

OK, I just pulled the modification. I just "merged" the notification queue. Root is still there.

barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
barche pushed a commit to barche/coolfluid3 that referenced this issue Oct 28, 2011
@barche
Copy link
Member

barche commented Oct 28, 2011

Root is now removed from common, and absolute paths are now like cpath:/Libraries, cpath:/Tools, i.e. they start with "/", where "/" always indicates the root, whatever its name is.

@gquentin, can you please look at commit feaacaa since I'm not sure about that one, though it fixed the test :)

@gquentin
Copy link
Member

Great job!!

Commit feaacaa seems OK. I will simplify this part since there is no Root anymore :)

There is only one thing that is not "correct" IMO: standard URI format is scheme://machine/path or (without the machine) scheme:///path . So we should have cpath:///Libraries. We discussed long time ago with Tiago about respecting this rule (but maybe things have changed since then) ;)

Anyway, this should not be a critical issue.

@tlmquintino
Copy link
Member Author

Yes. Quentin stands correct. According to the URI standard it should be:

scheme://machine/path

or

scheme:///path

I suggest we keep that.
In practice it means that scheme is followed by ://

@barche
Copy link
Member

barche commented Oct 29, 2011

OK. I suggest then to just insert the "//" when converting to string. For http, they are now stored in the path string, which complicates parsing.

wdeconinck added a commit to wdeconinck/coolfluid3 that referenced this issue Oct 31, 2011
* 'master' of github.com:coolfluid/coolfluid3:
  [FIX] Issue coolfluid#15: ntree utest
  [FIX] Temporarily disable lib cleanup because of issue coolfluid#145
  [DOC] Issue coolfluid#15: Update documentation after root removal
  [ENH] Issue coolfluid#15: remove root
  [FIX] Issue coolfluid#15: More fixes after rename of root to /
  [COSM] removed useless include
  [ENH] issue coolfluid#15 : removed event management from the Root. NotificationQueue now uses EventHandler.
  [DEV] Issue coolfluid#15: Fix some errors after rename
  [DEV] Issue coolfluid#15: Start renaming root
  [FIX] Make Component data members private
  [FIX] GUI graphics uTest
  [ENH] Issue coolfluid#48: complete_path is now implemented as access_component().uri()

Conflicts:
	cf3/common/Component.hpp
	cf3/common/URI.hpp
@tlmquintino
Copy link
Member Author

Can we close this issue?

@barche
Copy link
Member

barche commented Apr 18, 2012

no, the double slash is not there yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants