Packages/bundles that are already imported/added are shown #1195

lathapatil · 2024-03-13T11:57:05Z

Packages/bundles that are already imported/added are shown followed by [already imported/added]

Fixes #146

gireeshpunathil · 2024-03-13T11:59:20Z

@lathapatil - did change set from #1194 carry over into this PR?

lathapatil · 2024-03-13T12:05:06Z

@lathapatil - did change set from #1194 carry over into this PR?

No, I am trying to figure out what went wrong. Looks like #1194 has not reached here along with its branch !

github-actions · 2024-03-13T12:29:24Z

Test Results

279 files - 1 279 suites - 1 45m 15s ⏱️ - 6m 33s
3 586 tests ±0 3 509 ✅ - 1 76 💤 ±0 1 ❌ +1
10 756 runs - 2 10 553 ✅ - 3 202 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit 15a6239. ± Comparison against base commit b29e97a.

♻️ This comment has been updated with latest results.

gireeshpunathil · 2024-03-13T12:33:12Z

it is probably because you branched out for this PR from the previous branch, so the tip of the branch got carried over.

nothing to worry: I will help you fix that. if you are using command line then do this:

git rebase -I HEAD~2
this opens up the commit editor in vim editor
you will see 2 entries for the two commits.
remove the line that has 214071f in the commit hash
save and close
git push -f <origin branch> < this pr branch>

lathapatil · 2024-03-13T12:59:54Z

it is probably because you branched out for this PR from the previous branch, so the tip of the branch got carried over.

Yes. Thanks for the support .
I am using EGit and working in windows OS for PDE . I was able to fix this issue using EGit interactive rebase.

lathapatil · 2024-03-20T04:28:23Z

@laeubi Could you please test the PR if it meets the expectations and possibly review the code ?

laeubi · 2024-03-20T05:43:09Z

@lathapatil do I need to consider something special?

I fetched this PR then started an embedded workbench and tried the following:

but package aQute.bnd.build is not shown.

I also think it would be good to have a checkbox (enabled by default) "Show already imported packages", at best the user choice is persisted to survive a restart.

lathapatil · 2024-03-20T11:09:31Z

I fetched this PR then started an embedded workbench and tried the following:

After checking out the pull request (PR) while in PDE workspace , switch to the PR branch and launch the Eclipse application. Then, create a plugin project and inspect the manifest file for imported packages, required bundles, and exported packages.

This is what I have attempted, and the changes are visible.

What do you mean by embedded workbench ?

lathapatil · 2024-03-20T11:25:04Z

I also think it would be good to have a checkbox (enabled by default) "Show already imported packages"

This is already part of the PR. But I observed there is some delay in loading all packages

lathapatil · 2024-04-09T10:15:37Z

Please find attached video for working of the feature from the branch https://github.com/lathapatil/eclipse.pde/tree/Issues/146_Show_imported_exported_required_bundles

2024-04-09_15h43_11.mp4

lathapatil · 2024-09-18T05:45:37Z

@laeubi could you please try testing and reviewing this ?
@gireeshpunathil Can you please try to test this .. if you are also not able to see the changes

gireeshpunathil · 2024-09-19T11:48:21Z

@lathapatil - sure, will do. also, could you pls rebase your changes? as it was quite some time, now there is a conflict that has emerged.

lathapatil · 2024-10-22T11:00:55Z

@lathapatil - sure, will do. also, could you pls rebase your changes? as it was quite some time, now there is a conflict that has emerged.

Now it is rebased with conflicts resolved. Could you please verify the behavior

gireeshpunathil · 2024-10-22T12:37:44Z

@lathapatil - thanks, I tested this and found two minor issues. not sure if you want to fix it in this PR or another one:

when I select plugins or packages that are already in and not already in together, then the add button is ON. puzzled on what would be the right behaviour!

export packages section is showing empty, even when I have one. your video shows it right, however. so I am wondering what would be the difference between the environment of yours and mine!

Fixes eclipse-pde#146 Files not present in branch but changes are visible in github commit Missed code/files while Conflict resolution

lathapatil · 2024-10-23T06:29:36Z

puzzled on what would be the right behaviour!

in my opinion enabling ADD button is valid because user have selected one which is not added yet in the multiple selection and only that will be added as a result. It is mentioned here as well

export packages section is showing empty, even when I have one. your video shows it right, however. so I am wondering what would be the difference between the environment of yours and mine!

I found one bug in this and fixed please take latest commit .
I get some packages to export because I have some non-empty packages in my plugin project whose manifest file we are operating on.

gireeshpunathil · 2024-10-25T14:44:42Z

@lathapatil - I tested the new changes, and it works fine, as you have showed in the video. thanks!

HannesWell

Thanks for working on this.
I havn't done a technical/functional review but have a few style remarks below.

HannesWell · 2024-10-26T22:10:21Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/dialogs/PluginSelectionDialog.java

@@ -153,8 +159,8 @@ private static IPluginModelBase[] getElements(boolean includeFragments) {
 		return PluginRegistry.getActiveModels(includeFragments);
 	}

-	public static HashSet<String> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {
-		HashSet<String> existingImports = new HashSet<>();
+	public static HashMap<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {


Please use only collection interfaces (as abstract as possible) if possible here and in all other changed code. Using a specific implementation is usually unnecessarily restrictive.

Suggested change

public static HashMap<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {

public static Map<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {

HannesWell · 2024-10-26T22:19:47Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/PDELabelProvider.java

+
+		HashMap<String, Boolean> existingImports = PluginSelectionDialog.getExistingImports(currentModel, false);


Instead of setting IPluginModelBase currentModel via a setter and saving it, could you just use the following?

if (pluginBase.getModel() instanceof IPluginModelBase pluginModel) { ...

Because the less state we have the better. I also don't know if currentModel can be null at this point in any usage scenario and if the called PluginSelectionDialog.getExistingImports method would handle that well.

HannesWell · 2024-10-26T22:22:19Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/dialogs/PluginSelectionDialog.java

+			if (selection instanceof IPluginModelBase
+					&& !(existingImports.keySet().contains(((IPluginModelBase) selection).getPluginBase().getId()))) {


Please use pattern matching for instanceof in all changed code and if applicable.

Suggested change

if (selection instanceof IPluginModelBase

&& !(existingImports.keySet().contains(((IPluginModelBase) selection).getPluginBase().getId()))) {

if (selection instanceof IPluginModelBase pluginModel && !(existingImports.keySet().contains(pluginModel).getPluginBase().getId()))) {

Please also reformat the line, not sure if it fits in one line now.

HannesWell · 2024-10-26T22:25:15Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/dialogs/PluginSelectionDialog.java

-	public static HashSet<String> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {
-		HashSet<String> existingImports = new HashSet<>();
+	public static HashMap<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {
+		HashMap<String, Boolean> existingImports = new HashMap<>();


Maybe I oversaw something, but I can only see the key-set of the map being used and values seem not to be considered at all. If that's correct, can this Map stay a Set?

lathapatil force-pushed the Issues/146_Show_imported_exported_required_bundles branch from 6d51a60 to f0da0ed Compare March 13, 2024 12:56

lathapatil force-pushed the Issues/146_Show_imported_exported_required_bundles branch 2 times, most recently from aeb906f to 7b670f6 Compare October 22, 2024 10:55

Packages/bundles that are already imported/added are shown

15a6239

Fixes eclipse-pde#146 Files not present in branch but changes are visible in github commit Missed code/files while Conflict resolution

lathapatil force-pushed the Issues/146_Show_imported_exported_required_bundles branch from 8717f18 to 15a6239 Compare October 23, 2024 06:22

HannesWell reviewed Oct 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages/bundles that are already imported/added are shown #1195

Packages/bundles that are already imported/added are shown #1195

lathapatil commented Mar 13, 2024

gireeshpunathil commented Mar 13, 2024

lathapatil commented Mar 13, 2024

github-actions bot commented Mar 13, 2024 •

edited

Loading

gireeshpunathil commented Mar 13, 2024 •

edited

Loading

lathapatil commented Mar 13, 2024

lathapatil commented Mar 20, 2024

laeubi commented Mar 20, 2024

lathapatil commented Mar 20, 2024 •

edited

Loading

lathapatil commented Mar 20, 2024

lathapatil commented Apr 9, 2024

lathapatil commented Sep 18, 2024

gireeshpunathil commented Sep 19, 2024

lathapatil commented Oct 22, 2024

gireeshpunathil commented Oct 22, 2024

lathapatil commented Oct 23, 2024 •

edited

Loading

gireeshpunathil commented Oct 25, 2024

HannesWell left a comment

HannesWell Oct 26, 2024

HannesWell Oct 26, 2024

HannesWell Oct 26, 2024

HannesWell Oct 26, 2024

	public static HashMap<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {
	public static Map<String, Boolean> getExistingImports(IPluginModelBase model, boolean includeImportPkg) {


		HashMap<String, Boolean> existingImports = PluginSelectionDialog.getExistingImports(currentModel, false);

		if (selection instanceof IPluginModelBase
		&& !(existingImports.keySet().contains(((IPluginModelBase) selection).getPluginBase().getId()))) {

Packages/bundles that are already imported/added are shown #1195

Are you sure you want to change the base?

Packages/bundles that are already imported/added are shown #1195

Conversation

lathapatil commented Mar 13, 2024

gireeshpunathil commented Mar 13, 2024

lathapatil commented Mar 13, 2024

github-actions bot commented Mar 13, 2024 • edited Loading

Test Results

gireeshpunathil commented Mar 13, 2024 • edited Loading

lathapatil commented Mar 13, 2024

lathapatil commented Mar 20, 2024

laeubi commented Mar 20, 2024

lathapatil commented Mar 20, 2024 • edited Loading

lathapatil commented Mar 20, 2024

lathapatil commented Apr 9, 2024

lathapatil commented Sep 18, 2024

gireeshpunathil commented Sep 19, 2024

lathapatil commented Oct 22, 2024

gireeshpunathil commented Oct 22, 2024

lathapatil commented Oct 23, 2024 • edited Loading

gireeshpunathil commented Oct 25, 2024

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Oct 26, 2024

Choose a reason for hiding this comment

HannesWell Oct 26, 2024

Choose a reason for hiding this comment

HannesWell Oct 26, 2024

Choose a reason for hiding this comment

HannesWell Oct 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2024 •

edited

Loading

gireeshpunathil commented Mar 13, 2024 •

edited

Loading

lathapatil commented Mar 20, 2024 •

edited

Loading

lathapatil commented Oct 23, 2024 •

edited

Loading