BndEditor build.bnd: Plugin list should detect plugins by prefix #6069
Conversation
and finally shows plugins which are defined like -plugin.6.Central -plugin.7.Local Signed-off-by: Christoph Rueger <[email protected]>
now when you click on it you see a dialog with input fields Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
before this change, saving the build.bnd file after changing something in the editor added a single fixed "-plugin" property instead of keeping the existing ones: e.g. -plugin.6.Central, -plugin.7.Local Now the existing properties are kept / merged Signed-off-by: Christoph Rueger <[email protected]>
removal is a bit more complicated now, since we have a Map<String,List<>>. so we need to iterate over the value lists to find the right entry. Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
There was a problem hiding this comment.
Quite amazing you got this to work in so little code.
However, I think we need a generic solution in the BndEditModel for merged & inherited properties. Inherited properties should be greyed out but when you add properties it is important to realize they exist and what their value is. (Double clicking could open an editor on their file.)
It seems to screem for a MergedProperty class that handles this generically and can parameterize the type specific interactions? This is a common problem and I think it is a good idea to see how for example -buildpath would look like in this way?
Fantastic work! This has been a sore point forever.
| @@ -967,14 +969,67 @@ public void setTestSuites(List<String> suites) { | |||
| } | |||
|
|
|||
| public List<HeaderClause> getPlugins() { | |||
| return doGetObject(Constants.PLUGIN, headerClauseListConverter); | |||
| // return all plugins | |||
There was a problem hiding this comment.
Not sure if this works with inheritance of properties & bnd file properties? It is a powerful but nasty problem
file 1->* 'plugin.*' property 0->* clause
To make this work I think you need to model this as an object. When you edit a file, you can edit the local ones in the BndEditoModel but I think the inherited ones should be visible greyed out?
There was a problem hiding this comment.
I think we should have a call about this, so I do not misunderstand. The method getPlugins() and the new getPluginsProperties() I added work only locally.
It looks like I could do
Set<String> propertyKeys = getProperties().getPropertyKeys(true); to also get inherited properties (instead of getAllPropertyNames()).
To me it seems that the BndEditModel is designed for the local physical file you are looking at. If we would like to visualize inherited properties , we should talk about how this should look like. Greyed out in the same TableViewer could be an idea, but maybe it makes it more complicated, since you need to distinguish between local properties and inherited properties.
Maybe an alternative idea would be an additional (collapsible if possible) non-editable TableViewer which only shows inherited stuff? That way we could treat it separately. Or we have a button saying "Show inherited" and then then all inherited plugins appear too, but editing is disabled?
Anyway, maybe I am misunderstanding or thinking too complicated :)
There was a problem hiding this comment.
I have something in mind about your suggestion of MergedProperty class.
Are you basically talking about something like
public List<HeaderClause> getPlugins() {..}changing topublic List<MergedProperty> getPlugins() {..}
so that all the stuff which I currently do with the Map<String, List<HeaderClause>> happens under the hood + all the inheritance stuff?
where MergedProperty has something like:
String .getKey()returns the local property key e.g. -plugin.1.MavenCentralList<HeaderClause> .getHeaders()boolean .isInherited()- returnsfalseif it is only local.trueotherwiseList<MergedProperty> getParent()- returns the parent which is inherited
Something like this?
Can you also shed some more light in the meaning of the term merged here? I have the feeling I have not a good understanding of that at the moment. e.g. is merging referring to merging of the List<HeaderClause> when they are defined under the same property key?
| @@ -871,6 +871,23 @@ | |||
| description="Disable verification of server's X509 certificate (insecure, for testing only!)" /> | |||
| </plugin> | |||
|
|
|||
| <plugin class="aQute.bnd.repository.maven.provider.MavenBndRepository" | |||
There was a problem hiding this comment.
You mean those sections in the _plugin.xml ?
Yes, that is how I came to this PR? I was trying the +-Button of the Plugins list, because I wanted to see what this is... because I have never seen anything in the Plugins list of my build.bnd. And tadaaa I saw that I could add repositories. Unfortunately I did not see the MavenRepo we are using.
We can later add also support for other repositories if needed. The xml I created from aQute.bnd.repository.maven.provider.Configuration. I guess we can do the same for aQute.bnd.repository.maven.pom.provider.PomConfiguration.
this is a kinda hacky attempt to achieve using a new MergedHeaderClause object which "knows" if it is from a local file or inherited from an included file. The name MergedHeaderClause does not really fit, but I intent to clean it up later. Signed-off-by: Christoph Rueger <[email protected]>
|
@pkriens I have pushed another commit
The last two plugins are inherited from included This is a "hacky" attempt to implement
Although I created a class
But let's discuss based on this and make it better from there. |
| * @return a map with a property keys and their plugins. | ||
| */ | ||
| public Map<String, List<MergedHeaderClause>> getPluginsProperties() { | ||
| // return all plugins |
There was a problem hiding this comment.
I think this logic should be moved to Processor, next to mergeProperties so it is easier to track when we make changes in one or the others. Since this is processor, I think we need a call like:
Processor.getMergedParts(String keyStem) : MergedPart(Processor p, String keyStem, String actualKey, String value)
We could even rewrite ( one day) the mergedProperties with this method. In the BndEditModel, you then turn each one into a number of constituent clauses that can be edited if the processor == the BndEditModel's processor. This would be similar to your MergedHeaderClause, but I would make it something like:
MergedHeaderClause( MergedPart part, HeaderClause), `isLocal()` can check the part if it is the same Processor.
Signed-off-by: Christoph Rueger <[email protected]>
…d-detect-plugins-by-prefix
…d-detect-plugins-by-prefix
|
@pkriens I merged https://github.com/bndtools/bnd/pull/6073/files into master. One thing I noticed while trying to adapt the new stuff is this exception. I then see this. I can the close the |
|
@pkriens I experimented a bit with the Here is what I did in BndEditModel: in getPropertyKeys() I added some debug output inside the 
Important:
It seems that Why is this a problem?
The latter gives me "true" for everything physically in the That means, the new stuff does not give me an advantage over my previous approach. I was hoping the the new What's nextJust wanted to write this down. It helps me to think this more through.
|
|
Thinking out loud.... Adding to the above:
... so it contains the pure content of that file...without any inheritance or include loading. Maybe the |
|
I had the nagging feeling this would happen. The same is also true for the The only way to detect, as we had discussed, is to look in the edit model. It actually reflects the contents in the editor. So you cannot use isLocal in your case, the original method you had was better. I'll fix the NPE and see why it is null. |
|
I prefer not to add the information if a property is from the file or not. This would involve a lot of work and somehow it makes intuitive sense to use the BndEditModel for this since it is really on the file. Trying to track the pedigree of properties feels like a rats nest. |
Understandable and totally fine for me. But, the only thing I am wondering now:
Do you see use-cases where the new
Given this statement, I wonder if we should reevaluate the requirement to show greyed out (non-in-the-file) plugins in the plugin list. Although I can understand the wish to see them, I don't know if it somehow blurs the line between being
b) adds a certain complexity and derives from the current "what you see is what you get" behavior of the BndEditor. I don't want to discuss away what we have. I already have the 'greyed out' stuff implemented. I just see that it is more complex than the initial approach (only local stuff). |
- PropertyKey.filterVisible(List<PK> ) - show name + propertyKey.key in plugin list - hide plugin properties and show as tooltip instead Signed-off-by: Christoph Rueger <[email protected]>
…d-detect-plugins-by-prefix
adding new plugins was broken Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
we can now add a "helpUrl" attribute to the plugin in _plugin.xml which opens a browser with the website. so we allow to learn more about a plugin Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
let's at least show some helpful page in case a plugin author did not include a custom url Signed-off-by: Christoph Rueger <[email protected]>
Signed-off-by: Christoph Rueger <[email protected]>
- so that it appears in the UI - improve some default values - sort MavenBndRepo as first Signed-off-by: Christoph Rueger <[email protected]>
|
I did the following additions:
I thinks this is ready for review / merge now. @pkriens |
Signed-off-by: Christoph Rueger <[email protected]>
|
Hi @pkriens , how do we want to proceed here? |
|
Hi, I haven't forgotten you. However, I spent a lot of time making the BndEditModel clean. I have a test case in the resolver where I can't figure out why I get 4 bundles in the old case and only 2 in the new case. The Processor is identical and the debug trace is identical :-( Drives me crazy. Then there was an issue with project creation which I urgently needed to fix. I am ok to merge this. I will then take this and integrate the new BndEditModel and then we can see where we are. Ok? |
|
@pkriens no problem. Merging is totally fine if you don't see any obvious things. Thanks. |
|
Thanks Peter. |
Currently the Plugin list is empty when plugins are defined like:
This PR is supposed to change it, so that it becomes:
Additionally the MavenBndRepository should be a proper editor like this: