eclipse-platform / eclipse.platform.resources

Eclipse Public License 2.0
3 stars 18 forks source link

Cyclic dependency between ProjectPreferences and Workspace init #87

Closed laeubi closed 2 years ago

laeubi commented 2 years ago

Fix https://github.com/eclipse-platform/eclipse.platform.resources/issues/86

Its not that nice because we use deprecated Preferences API here, but this prevents the implicit early Workspace access and is consistent to how ResourcesPlugin.getEncoding() and WorkspaceRoot.getDefaultCharset(boolean) behaves here.

Actually it would be good if none would ever call ResourcesPlugin.getEncoding() but instead Workspace.getRoot().getDefaultCharset() but unless this do not change we should simply keep this behavior here.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-87/1/

vogella commented 2 years ago

So any activator which uses regular preferences API may cause the same issue?

laeubi commented 2 years ago

So any activator which uses regular preferences API may cause the same issue?

No that is a bit different as it will trigger activation of the Resource plugin and then "wait" for Workspace init (hopefully this does not take to long ...) while in this case we are in the (internal) init phase so should avoid relying to much on other parts that indirectly "recall".

Anyways I opened https://github.com/eclipse-equinox/equinox.bundles/issues/24 to make this more declarative and do not require plugins to use activator at all or call static methods for this common use-case.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-87/2/

laeubi commented 2 years ago

Old order should be preserved now (just moved the save a bit above), looking at the code it feels like both statements should be moved down throw new ResourceException .. strange enough there is already a WorkspacePreferences object created maybe we better should use that anyways??

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-87/3/

iloveeclipse commented 2 years ago

Old order should be preserved now (just moved the save a bit above),

thanks.

looking at the code it feels like both statements should be moved down throw new ResourceException

That was my original proposal (to move preferences order init after setting encoding)

.. strange enough there is already a WorkspacePreferences object created maybe we better should use that anyways??

The WorkspacePreferences is a "legacy wrapper" to few preference values that were previously in an xml file, see https://github.com/eclipse-platform/eclipse.platform.resources/pull/93

laeubi commented 2 years ago

So probably it would make sense to first getting #93 done and then rebase on that change?

iloveeclipse commented 2 years ago

So probably it would make sense to first getting #93 done and then rebase on that change?

Sure, that's the reason you are set as reviewer on PR :-)

laeubi commented 2 years ago

Rebased now.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-87/4/

iloveeclipse commented 2 years ago

Thanks, looks good, I'm looking into it now once again, just to be sure we don't change any logic here.