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

BndEditor build.bnd: Plugin list should detect plugins by prefix #6069

Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion biz.aQute.bndlib/src/aQute/bnd/build/model/BndEditModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Properties;
import java.util.Set;
import java.util.TreeMap;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

Expand Down Expand Up @@ -967,14 +969,117 @@ public void setTestSuites(List<String> suites) {
}

public List<HeaderClause> getPlugins() {
return doGetObject(Constants.PLUGIN, headerClauseListConverter);
// return all plugins
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have something in mind about your suggestion of MergedProperty class.
Are you basically talking about something like

  • public List<HeaderClause> getPlugins() {..} changing to public 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.MavenCentral
  • List<HeaderClause> .getHeaders()
  • boolean .isInherited() - returns false if it is only local. true otherwise
  • List<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?

// we do prefix matching to support merged properties like
// -plugin.1.Test, -plugin.2.Maven etc.
try {
Processor proc = getProperties();
Properties allProps = proc.getProperties();
Set<String> propertyKeys = proc.getPropertyKeys(true);

List<HeaderClause> headers = propertyKeys.stream()
.filter(p -> p.startsWith(Constants.PLUGIN))
.map(p -> headerClauseListConverter.convert(allProps.getProperty(p)))
.flatMap(List::stream)
.toList();

return headers;

} catch (Exception e) {
throw Exceptions.duck(e);
}

}

public void setPlugins(List<HeaderClause> plugins) {
List<HeaderClause> old = getPlugins();
doSetObject(Constants.PLUGIN, old, plugins, complexHeaderClauseListFormatter);
}

/**
* Similar to {@link #getPlugins()} but returns a map where the key is the
* property key of the bnd file e.g.
* <code>-plugin.1.Test, -plugin.2.Maven </code> The value is a List of
* plugins, although usually it is just a 1-element list. But it is also
* possible to specify multiple plugins under a single key, thus it is a
* list.
*
* @return a map with a property keys and their plugins.
*/
public Map<String, List<MergedHeaderClause>> getPluginsProperties() {
// return all plugins
Copy link
Member

Choose a reason for hiding this comment

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

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.

// we do prefix matching to support merged properties like
// -plugin.1.Test, -plugin.2.Maven etc.

try {
Processor proc = getProperties();
Properties allProps = proc.getProperties();
Set<String> propertyKeys = proc.getPropertyKeys(true);


Map<String, List<MergedHeaderClause>> map = new LinkedHashMap<>();

propertyKeys.stream()
.filter(p -> p.startsWith(Constants.PLUGIN))
.forEach(key -> {

boolean isLocal = doGetObject(key, headerClauseListConverter) != null;

List<HeaderClause> headers = headerClauseListConverter.convert(allProps.getProperty(key));
map.put(key, headers.stream()
.map(h -> new MergedHeaderClause(key, h, isLocal))
.collect(Collectors.toList()));

});

return map;

} catch (Exception e) {
throw Exceptions.duck(e);
}

}

/**
* Updates and removes plugins.
*
* @param plugins
* @param pluginPropKeysToRemove the property keys to remove (not modified,
* caller needs to handle cleanup)
*/
public void setPlugins(Map<String, List<MergedHeaderClause>> plugins, Collection<String> pluginPropKeysToRemove) {
Map<String, List<MergedHeaderClause>> old = getPluginsProperties();

plugins.entrySet()
.forEach(p -> {
if (!p.getKey()
.startsWith(Constants.PLUGIN)) {
throw new IllegalArgumentException(
"Plugin properties need to start with " + Constants.PLUGIN + ". Actual: " + p.getKey());
}

List<HeaderClause> newLocalHeaders = p.getValue()
.stream()
.filter(mh -> mh.isLocal())
.map(mh -> mh.header())
.toList();

List<HeaderClause> oldLocalHeaders = old.get(p.getKey())
.stream()
.filter(mh -> mh.isLocal())
.map(mh -> mh.header())
.toList();

doSetObject(p.getKey(), oldLocalHeaders, newLocalHeaders,
complexHeaderClauseListFormatter);

});

if (pluginPropKeysToRemove != null) {
pluginPropKeysToRemove.forEach(key -> removeEntries(key));
}
}

public List<String> getPluginPath() {
return doGetObject(Constants.PLUGINPATH, listConverter);
}
Expand Down
26 changes: 26 additions & 0 deletions biz.aQute.bndlib/src/aQute/bnd/build/model/MergedHeaderClause.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package aQute.bnd.build.model;

import java.util.Objects;

import aQute.bnd.build.model.clauses.HeaderClause;

public record MergedHeaderClause(String key, HeaderClause header, boolean isLocal) {

@Override
public int hashCode() {
return Objects.hash(header);
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
MergedHeaderClause other = (MergedHeaderClause) obj;
return Objects.equals(header, other.header);
}

}
17 changes: 17 additions & 0 deletions bndtools.core/_plugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

I had no idea this existed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

icon="icons/database.png"
name="MavenBndRepository">
<property name="name" type="string" description="The name of this repository" />
<property name="releaseUrl" type="string" description="The urls to the remote release repository." />
<property name="stagingUrl" type="string" description="The url of the staging release repository." />
<property name="snapshotUrl" type="string" description="The urls to the remote snapshot repository." />
<property name="local" type="string" description="The path to the local repository" default="~/.m2/repository" />
<property name="readOnly" type="boolean" description="" default="false" />
<property name="index" type="string" description="The path to the index file" />
<property name="source" type="string" description="Content added to the index file. Content maybe one line without CR/LF as long as there is a comma or whitespace separating the GAVs. Further same format as the index file." />
<property name="noupdateOnRelease" type="boolean" description="Do not update the index when a file is released" />
<property name="poll_time" type="int" description="Sets the time in seconds when to check for changes in the pom-files" default="5" />
<property name="redeploy" type="boolean" description="Allow redeploy" />
<property name="ignore_metainf_maven" type="boolean" description="Ignore maven information in META-INF/maven/...." />
<property name="multi" type="string" description="Extensions for files that contain multiple JARs" />
</plugin>
</extension>

<extension point="org.eclipse.ui.intro.configExtension">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.eclipse.swt.graphics.Image;
import org.eclipse.ui.plugin.AbstractUIPlugin;

import aQute.bnd.build.model.MergedHeaderClause;
import aQute.bnd.build.model.clauses.HeaderClause;
import bndtools.Plugin;

Expand All @@ -27,10 +28,12 @@ public PluginClauseLabelProvider(Map<String, IConfigurationElement> configElemen

@Override
public void update(ViewerCell cell) {
HeaderClause header = (HeaderClause) cell.getElement();
MergedHeaderClause mh = (MergedHeaderClause) cell.getElement();
HeaderClause header = mh.header();

String className = header.getName();
StyledString label = new StyledString(className);

StyledString label = new StyledString(className, mh.isLocal() ? null : StyledString.QUALIFIER_STYLER);

Map<String, String> attribs = header.getAttribs();
if (!attribs.isEmpty())
Expand Down Expand Up @@ -75,6 +78,14 @@ public void update(ViewerCell cell) {
cell.setImage(image);
}

@Override
public String getToolTipText(Object element) {
if (element instanceof MergedHeaderClause mh && !mh.isLocal()) {
return "This inherited plugin can only be edited in the included .bnd file.";
}
return super.getToolTipText(element);
}

@Override
public void dispose() {
super.dispose();
Expand Down
86 changes: 75 additions & 11 deletions bndtools.core/src/bndtools/editor/workspace/PluginsPart.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import org.bndtools.core.ui.icons.Icons;
import org.eclipse.core.resources.IFile;
Expand All @@ -18,6 +22,7 @@
import org.eclipse.core.runtime.Platform;
import org.eclipse.core.runtime.Status;
import org.eclipse.jface.viewers.ArrayContentProvider;
import org.eclipse.jface.viewers.ColumnViewerToolTipSupport;
import org.eclipse.jface.viewers.ISelectionProvider;
import org.eclipse.jface.viewers.IStructuredSelection;
import org.eclipse.jface.viewers.TableViewer;
Expand Down Expand Up @@ -46,6 +51,7 @@
import org.eclipse.ui.ide.ResourceUtil;

import aQute.bnd.build.model.BndEditModel;
import aQute.bnd.build.model.MergedHeaderClause;
import aQute.bnd.build.model.clauses.HeaderClause;
import aQute.bnd.header.Attrs;
import aQute.bnd.osgi.Constants;
Expand All @@ -58,7 +64,8 @@ public class PluginsPart extends SectionPart implements PropertyChangeListener {

private final Map<String, IConfigurationElement> configElements = new HashMap<>();

private List<HeaderClause> data;
private Map<String, List<MergedHeaderClause>> data;
private Set<String> pluginsPropertiesToRemove = new LinkedHashSet<>();

private Table table;
private TableViewer viewer;
Expand All @@ -68,6 +75,7 @@ public class PluginsPart extends SectionPart implements PropertyChangeListener {

private BndEditModel model;


public PluginsPart(Composite parent, FormToolkit toolkit, int style) {
super(parent, toolkit, style);

Expand All @@ -91,6 +99,7 @@ final void createSection(Section section, FormToolkit toolkit) {
table = toolkit.createTable(composite, SWT.FULL_SELECTION | SWT.SINGLE | SWT.BORDER);

viewer = new TableViewer(table);
ColumnViewerToolTipSupport.enableFor(viewer);
viewer.setContentProvider(ArrayContentProvider.getInstance());
viewer.setLabelProvider(new PluginClauseLabelProvider(configElements));

Expand Down Expand Up @@ -206,19 +215,23 @@ public void dispose() {

@Override
public void refresh() {
List<HeaderClause> modelData = model.getPlugins();
Map<String, List<MergedHeaderClause>> modelData = model.getPluginsProperties();
if (modelData != null)
this.data = new ArrayList<>(modelData);
this.data = new LinkedHashMap<>(modelData);
else
this.data = new LinkedList<>();
viewer.setInput(this.data);
this.data = new LinkedHashMap<>();
viewer.setInput(this.data.values()
.stream()
.flatMap(List::stream)
.toList());
super.refresh();
}

@Override
public void commit(boolean onSave) {
super.commit(onSave);
model.setPlugins(data);
model.setPlugins(data, pluginsPropertiesToRemove);
pluginsPropertiesToRemove.clear();
}

@Override
Expand All @@ -238,14 +251,32 @@ void doAdd() {
if (dialog.open() == Window.OK) {
HeaderClause newPlugin = wizard.getHeader();

data.add(newPlugin);
String uniqueKey = uniqueKey(Constants.PLUGIN);
data.put(uniqueKey, Collections.singletonList(new MergedHeaderClause(uniqueKey, newPlugin, true)));
viewer.add(newPlugin);
markDirty();
}
}

private String uniqueKey(String key) {
String newKey = key;
int i = 1;
while (data.containsKey(newKey)) {
newKey = key + "." + i;
i++;
}
return newKey;
}

void doEdit() {
HeaderClause header = (HeaderClause) ((IStructuredSelection) viewer.getSelection()).getFirstElement();
MergedHeaderClause mh = (MergedHeaderClause) ((IStructuredSelection) viewer.getSelection()).getFirstElement();
HeaderClause header = mh.header();

if (!mh.isLocal()) {
// only local plugins in this file can be edited
return;
}

if (header != null) {
Attrs copyOfProperties = new Attrs(header.getAttribs());

Expand All @@ -267,10 +298,43 @@ void doEdit() {
}

void doRemove() {

IStructuredSelection sel = (IStructuredSelection) viewer.getSelection();

MergedHeaderClause mh = (MergedHeaderClause) sel.getFirstElement();
HeaderClause header = mh.header();

if (!mh.isLocal()) {
// only local plugins in this file can be removed
return;
}

viewer.remove(sel.toArray());
data.removeAll(sel.toList());

// remove by value
sel.toList()
.forEach(selectedPlugin -> {
Set<Entry<String, List<MergedHeaderClause>>> entrySet = data.entrySet();
inner: for (Iterator<Entry<String, List<MergedHeaderClause>>> iterator = entrySet.iterator(); iterator
.hasNext();) {
Entry<String, List<MergedHeaderClause>> entry = iterator.next();
String key = entry.getKey();
List<MergedHeaderClause> headers = entry.getValue();

boolean removed = headers.removeIf(selectedPlugin::equals);
if (removed) {

if (headers.isEmpty()) {
// remove the map entry too
iterator.remove();
this.pluginsPropertiesToRemove.add(key);
}

// stop when we have removed the plugin
break inner;
}
}
});

if (!sel.isEmpty())
markDirty();
Expand Down
Loading