Bug 341910

Comment 1 Miles Parker 2012-02-14 22:15:01 EST

OK, we're getting close and closer. Leo, we've already dicussed much of this, but can you scan a look at this latest round for a sanity check? There are a lot of changes that have some potential to affect functionality here.

http://git.eclipse.org/c/virgo/org.eclipse.virgo.ide.git/commit/?id=e48849fb634dec15d840067e7b8660472f5d591a

Comment 2 Leo Dos Santos 2012-02-15 15:01:57 EST

Great, we're most of the way there! A few notes:

* In SpringCorePreferences the getters & setters prefix the key with a namespace. Your pref changes don't do that, so it will probably break existing preferences. Your call if you want to support that (it shouldn't be difficult), but if not then you need to make a note of it on the migration wiki.
* In BundlorProjectBuilder we can keep the reference to FileSystemPropertiesSource since that class comes from Virgo Bundlor. You've done a great job simplifying the createPropertiesSource() method, there's no reason why you should have to open a FileReader when you can construct a FileSystemPropertiesSource from a java.io.File
* In BundleOverviewPage line 241, there's no null check on resource, and this is breaking our test BundleManifestEditorTest.testCreateSystemFileContextsFromDotSettings
* The BundlorPreferencePage doesn't appear for me in Eclipse preferences or project properties anymore. We'll need to create a properties category for Virgo or Bundlor anyway, since that page currently gets added to a Spring IDE category

Comment 3 Miles Parker 2012-02-15 16:22:39 EST

Clearly I've broken something as well, as the tests are no longer passing. :) So some work to do.

(In reply to comment #2)
> * In SpringCorePreferences the getters & setters prefix the key with a
> namespace. Your pref changes don't do that, so it will probably break existing
> preferences. Your call if you want to support that (it shouldn't be difficult),
> but if not then you need to make a note of it on the migration wiki.

Yeah, I wasn't sure on that one. I checked for one project and they looked right for me. Do you mean that they had the project id appended before? I don't think that's standard practice for eclipse, if so.

> * In BundlorProjectBuilder we can keep the reference to
> FileSystemPropertiesSource since that class comes from Virgo Bundlor. You've
> done a great job simplifying the createPropertiesSource() method, there's no
> reason why you should have to open a FileReader when you can construct a
> FileSystemPropertiesSource from a java.io.File

Good call -- I thought that was in spring core.

> * In BundleOverviewPage line 241, there's no null check on resource, and this is
> breaking our test
> BundleManifestEditorTest.testCreateSystemFileContextsFromDotSettings

That's the one I'm seeing. I'm not able to get the unit tests running locally yet..it's just an annoying plugin dependency issue that should be not too hard to fix I think, but for the moent that means that I don't see these until they hit hudson. :(

> * The BundlorPreferencePage doesn't appear for me in Eclipse preferences or
> project properties anymore. We'll need to create a properties category for Virgo
> or Bundlor anyway, since that page currently gets added to a Spring IDE category

If you mean that "Manifest generation" page, this does appear (under Spring, which we need to fix when we revisit the externalize strings bug) for projects, but only if you have the org.eclipse.virgo.ide.facet.core.bundlenature activated. (Can you confirm that?) But you're right, it doesn't show up in the workbench preferences.

Comment 4 Leo Dos Santos 2012-02-15 17:30:55 EST

(In reply to comment #3)
> Yeah, I wasn't sure on that one. I checked for one project and they looked right
> for me. Do you mean that they had the project id appended before? I don't think
> that's standard practice for eclipse, if so.

That's right, but prefix being passed in was the project id, and those changed from dm Server Tools -> Virgo tools, so now I'm thinking that preference migration never worked in the first place. We ought to make a note of that in the migration wiki and let people know that they'll have to set up their preferences.

> If you mean that "Manifest generation" page, this does appear (under Spring,
> which we need to fix when we revisit the externalize strings bug) for projects,
> but only if you have the org.eclipse.virgo.ide.facet.core.bundlenature
> activated. (Can you confirm that?) But you're right, it doesn't show up in the
> workbench preferences.

You're right. I see the page in project properties too, just not in preferences.

Comment 5 Miles Parker 2012-02-16 18:51:50 EST

We're painfully close now. We stiff have some par related stuff in virgo.ide.ui.

Comment 6 Miles Parker 2012-03-07 23:05:54 EST

OK, closed!