Bug 513735 - Launch Configurations View
Summary: Launch Configurations View
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 4.7   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.22 M2   Edit
Assignee: Markus Duft CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy, plan
Depends on: 577973 576225 577743 577958 577969
Blocks:
  Show dependency tree
 
Reported: 2017-03-16 02:46 EDT by Markus Duft CLA
Modified: 2022-01-20 05:14 EST (History)
10 users (show)

See Also:


Attachments
Launch Config View after relaunch (26.99 KB, image/png)
2017-03-24 02:01 EDT, Sarika Sinha CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Duft CLA 2017-03-16 02:46:17 EDT
Within the LcDsl (Launch Configuration DSL) project [1], we have published (and rewritten) a view that we have been using internally at SSI Schaefer for some years now.

This view is very simple, and allows to display and control launch configurations. It supports platform.debug ILaunchConfiguration out of the box, but can be extended by providers like LcDsl to display launch configurations that work differently.

Please see the LcDsl project page [1] for a small demonstration video of how the view works.

My suggestion is to integrate the view into platform.debug directly. It has no dependencies (except platform.debug itself) as it stands. The license is EPL already, and I'm willing to put a little more work into moving it here :)

The actual code is contained in this plugin (entirely) [2].

[1] https://github.com/mduft/lcdsl
[2] https://github.com/mduft/lcdsl/tree/master/com.wamas.ide.launchview
Comment 1 Mickael Istria CLA 2017-03-16 03:31:41 EDT
I've recently installed this plugin providing the Launch Configuration view and find it pretty useful. I would love to see it shipped by default in Platform as I believe it can be useful to many people.
Comment 2 Mickael Istria CLA 2017-03-16 03:37:59 EDT
@Markus: Because of schedule and deadlines, it's most likely something that cannot be part of 4.7/Oxygen now. So the 1st possible target for inclusion would be Photon.M1, which should happen in the summer IIRC.
The current master branch is for Oxygen and I believe it cannot include this new piece of code right now (for reason mentioned above). I suggest the following strategy:
1. you keep working on the GitHub repo
2. As soon as master is ready for Photon (in June), you submit this plugin as a Gerrit patch for platform.releng repo
3. we review it
4. it gets merged and included in 4.8.M1

Being included in 4.8.M1 doesn't enforce to put requirements on other 4.8 bundles. So unless you use new stuff, the bundle would still be backward compatible and could be included in older IDEs.

WDYT?
Comment 3 Mickael Istria CLA 2017-03-16 03:53:28 EDT
ACtually, I might be wrong: M7 still allows to add new features; so maybe it's doable in the next weeks?
@Project leads: WDYT?
Also, another question is: would this feature fit best in a separate bundle or as an addition in one of the existing bundles?
Comment 4 Markus Duft CLA 2017-03-16 04:34:26 EDT
The question about having it in a separate bundle is an important one IMO. Although it would be nice to see it in Oxygen already, I did not expect to have a chance to contribute it that early. If it would be possible, I'd put pressure on preparing the patches though - shouldn't take longer than a few days
Comment 5 Chandrayya CLA 2017-03-16 04:45:21 EDT
Its very helpful. Also eclipse runner plugin https://github.com/zaunerc/eclipserunnerplugin.
Comment 6 Markus Duft CLA 2017-03-16 04:51:24 EDT
I did not know about eclipserunner :) It looks quite nice. Maybe the author would be willing to contribute features to the view once we have it in platform.debug?
Comment 7 Sarika Sinha CLA 2017-03-16 23:57:13 EDT
4.7 looks practically difficult, one option could be to put it in market place for 4.7. It can be reviewed/tested so that by 4.8 M1 we will have much better idea on how to proceed.
Comment 8 Markus Duft CLA 2017-03-17 07:06:22 EDT
Created a marketplace entry for now: https://marketplace.eclipse.org/content/launch-configuration-dsl

This entry is for the Launch Configuration DSL. The view is currently hosted along with the DSL. Should/can I create a separate entry for the view ONLY?
Comment 9 Sarika Sinha CLA 2017-03-20 00:28:43 EDT
(In reply to Markus Duft from comment #8)
> Created a marketplace entry for now:
> https://marketplace.eclipse.org/content/launch-configuration-dsl
> 
> This entry is for the Launch Configuration DSL. The view is currently hosted
> along with the DSL. Should/can I create a separate entry for the view ONLY?

Yes, a separate entry for views only will be helpful if it's not much of an effort to separate it out.
Comment 10 Markus Duft CLA 2017-03-20 03:52:16 EDT
Separate entry for the view is here: https://marketplace.eclipse.org/content/launch-configuration-view
Comment 11 Sarika Sinha CLA 2017-03-21 02:09:09 EDT
Can we have the gerrit patch for this ?
Comment 12 Markus Duft CLA 2017-03-21 09:03:55 EDT
Sure, I can start working on it - currently it's all branded com.wamas and not org.eclipse ;) Should it be in a separate bundle?
Comment 13 Sarika Sinha CLA 2017-03-23 03:02:22 EDT
I installed "Launch Configuration View" from Market Place. 
Few quick observations -
1. Relaunch - terminates the current launch, from debug view, relaunch creates a new launch.
2. After Relaunch, the Launch Configuration View does not show the Launch config as running with the green arrow on it and can't terminate.
Comment 14 Markus Duft CLA 2017-03-23 03:13:56 EDT
First of all thanks for taking a look at it!

I don't really understand your problem (yet). I tested with a simple java application that just sleeps for 30 seconds. When I press relaunch, the launch is terminated and re-launched. The implementation is exactly as if first pressing terminate and afterwards run/debug/... The point here is that the implementation of the view uses a dedicated job to launch, providing a better UX (in our experience). After the application has re-launched, I get the green arrow and can terminate.

What kind of application did you try? Is the launch configuration a standard eclipse configuration, or did you also try LcDsl in your test?

As always, there must be more to this :)
Comment 15 Eclipse Genie CLA 2017-03-23 05:53:36 EDT
New Gerrit change created: https://git.eclipse.org/r/93689
Comment 16 Markus Duft CLA 2017-03-23 05:54:17 EDT
(In reply to Eclipse Genie from comment #15)
> New Gerrit change created: https://git.eclipse.org/r/93689

I'm pretty sure there are still rough edges, please review so I can continue to refine it :)
Comment 17 Sarika Sinha CLA 2017-03-24 01:58:33 EDT
(In reply to Markus Duft from comment #14)
> First of all thanks for taking a look at it!
> 
> I don't really understand your problem (yet). I tested with a simple java
> application that just sleeps for 30 seconds. When I press relaunch, the
> launch is terminated and re-launched. The implementation is exactly as if
> first pressing terminate and afterwards run/debug/... The point here is that
> the implementation of the view uses a dedicated job to launch, providing a
> better UX (in our experience). After the application has re-launched, I get
> the green arrow and can terminate.
> 
> What kind of application did you try? Is the launch configuration a standard
> eclipse configuration, or did you also try LcDsl in your test?
> 
> As always, there must be more to this :)

Relaunch from Debug View does not terminate the current launch.
I tried a simple java application.
Attaching the screenshot for not displaying green arrow
Comment 18 Sarika Sinha CLA 2017-03-24 02:01:02 EDT
Created attachment 267442 [details]
Launch Config View after relaunch
Comment 19 Markus Duft CLA 2017-03-24 04:11:01 EDT
Regarding the terminate when pressing relaunch, I think there is a historically different interpretation... Here at my company we wanted a button to restart a program from scratch - that's this relaunch. It does something different then relaunch in the debug view intentionally. I think I should somehow rename the action then... "Terminate & Relaunch"?

(In reply to Sarika Sinha from comment #18)
> Created attachment 267442 [details]
> Launch Config View after relaunch

Thanks, I'll have a look at this :)
Comment 20 Markus Duft CLA 2017-03-24 04:18:42 EDT
(In reply to Sarika Sinha from comment #18)
> Created attachment 267442 [details]
> Launch Config View after relaunch

This is fixed both in the lcdsl repo, in the draft as well as on the p2 update site. Moving on to the relaunch action :)
Comment 21 Markus Duft CLA 2017-03-24 04:41:22 EDT
I updated the action to read "Terminate & Restart" and changed the icon. While at it, i made sure that all icons are scalable (@2x) where possible. Any way to get a larger version of the favorite_star.png icon? Or something else I should use for favorites?
Comment 22 Markus Duft CLA 2017-04-07 03:28:20 EDT
I just pushed another update. I think the contribution is now ready from my POV. I introduced NLS throughout the code, ran all cleanups/formatters, and generally copied all project specific settings from debug.ui, so that all validation and constraints are the same.

May I ask for review, CQ, whatever is necessary :)
Comment 23 Sarika Sinha CLA 2017-04-07 04:32:21 EDT
As it is not for 4.7, no CQ required. I will be able to look at it in June after 4.7 is almost done.
Comment 24 Dani Megert CLA 2017-04-07 04:35:47 EDT
(In reply to Sarika Sinha from comment #23)
> As it is not for 4.7, no CQ required.

That really depends on the size of the change.
Comment 25 Sarika Sinha CLA 2017-04-07 04:45:40 EDT
(In reply to Dani Megert from comment #24)
> (In reply to Sarika Sinha from comment #23)
> > As it is not for 4.7, no CQ required.
> 
> That really depends on the size of the change.

Yes, Thanks for clarification - I meant for now in 4.7.
Comment 26 Mickael Istria CLA 2017-07-03 09:47:14 EDT
I did try the patch and put a few comments on the Gerrit regarding code. The provided feature seems to work very well, and I still think it's a good addition to Eclipse Debug.
In the proposed patch, only the bundle is introduced, but it's not part of the default feature, so at first sight, end-users wouldn't be able to find this bundle on the p2 repo. I believe this makes a good 1st iteration for this patch, and we can consider adding it to the feature, or decide to put it in another feature, later.
In the current state, it seems to be a good candidate for 4.8.M1
@Sarika: feel free to ask if there is anything I can help with to ease and speed up the review and merge.
Comment 27 Sarika Sinha CLA 2017-07-18 05:54:40 EDT
I asked some Eclipse users to try the market place entry 
https://marketplace.eclipse.org/content/launch-configuration-view

But unfortunately, people are not seeing too much of an advantage that it should be a part of Platform. Suggestions came like may be adding a shortcut to launch Run/Debug Configuration view will help in most of the scenarios.

It's really good to have it it market place and we can publicize this so that people can install.
Comment 28 Markus Duft CLA 2017-08-03 02:00:57 EDT
Let's see how things are going :)

The view (for me personally) is most useful in combination with LcDsl - when extended in some way through it's extension mechanism, opening up to and centralising/grouping/managing contributions to launch configurations. I can understand that standalone seems like little improvement. Although - still - there are two aspects to that from my POV:

 1) The more launch configurations one has, the more usefull this view gets.
    (we have hundreds, and we are literally lost without that view ;)). After
    my talk at EclipseCon the people with LOTS of configurations came up to
    me and asked me to contribute this.
 2) The view itself does not have so much code to maintain (and rather simple
    code after all). I think having the possibility to extend in that area
    would be worth it - but that's not up to me to judge ;)
Comment 29 Sarika Sinha CLA 2018-04-13 02:55:59 EDT
Moving out of 4.8.
Comment 30 Markus Duft CLA 2019-08-28 04:05:04 EDT
There is some quite positive feedback on the marketplace, although not really a lot. We are still using this in production for hundreds of developers, and it is an essential part of Eclipse for us. Any ideas on how to proceed?
Comment 31 Eclipse Genie CLA 2019-09-11 01:26:39 EDT
New Gerrit change created: https://git.eclipse.org/r/93689
Comment 32 Lars Vogel CLA 2019-12-16 08:36:30 EST
*** Bug 532000 has been marked as a duplicate of this bug. ***
Comment 33 Lars Vogel CLA 2019-12-16 11:22:23 EST
Would be great to have this in 4.15
Comment 34 Sarika Sinha CLA 2020-05-13 05:23:00 EDT
Please assign the target milestone, once you have it ready.
Comment 35 Marco Descher CLA 2020-09-02 03:56:31 EDT
https://bugs.eclipse.org/bugs/show_bug.cgi?id=532000 is mentioned as a duplicate of this - but I don't see where, in this view, I am allowed to edit launch configurations in a non-modal way!?
Comment 36 Sarika Sinha CLA 2021-09-17 07:55:43 EDT
@Lars,
Good to see that you are driving this feature.
Please clarify, who will be taking care of the bugs which will be found in the new view after the release of this feature? As none of us know the internals and Markus Duft has stated that he does not have time.
Comment 37 Lars Vogel CLA 2021-09-17 07:59:21 EDT
(In reply to Sarika Sinha from comment #36)
> @Lars,
> Good to see that you are driving this feature.
> Please clarify, who will be taking care of the bugs which will be found in
> the new view after the release of this feature? As none of us know the
> internals and Markus Duft has stated that he does not have time.

Why special treatment here? This will just follow the normal development process, we ask Markus to help with bugs, if he is not available, we will try to fix issues, if that does not work, we may have to revert the commits.
Comment 38 Markus Duft CLA 2021-09-17 08:13:30 EDT
(In reply to Sarika Sinha from comment #36)
> @Lars,
> Good to see that you are driving this feature.
> Please clarify, who will be taking care of the bugs which will be found in
> the new view after the release of this feature? As none of us know the
> internals and Markus Duft has stated that he does not have time.

Since we're prime user of this view, and I did all the maintenance required in the past year right here on this change, I will also help in the future as much as time permits. I just don't have time for major refactorings and reworks. If an issue arises with the current code, I'm happy to help in one or the other way.
Comment 39 Sarika Sinha CLA 2021-09-17 09:12:08 EDT
(In reply to Markus Duft from comment #38)
> (In reply to Sarika Sinha from comment #36)
> > @Lars,
> > Good to see that you are driving this feature.
> > Please clarify, who will be taking care of the bugs which will be found in
> > the new view after the release of this feature? As none of us know the
> > internals and Markus Duft has stated that he does not have time.
> 
> Since we're prime user of this view, and I did all the maintenance required
> in the past year right here on this change, I will also help in the future
> as much as time permits. I just don't have time for major refactorings and
> reworks. If an issue arises with the current code, I'm happy to help in one
> or the other way.

Thanks Markus!
Comment 41 Lars Vogel CLA 2021-09-17 09:25:11 EDT
Thanks Markus. Can you add this to the N&N?
Comment 42 Andrey Loskutov CLA 2021-09-17 09:57:38 EDT
Immediate issues I've seen:

1) Double click launches the config. I've expected it would "Edit" config. May be that should have a toggle like in the History view (Double click edits or starts execution).

2) No tests at all. Not even smoke tests that the view can be started.

3) Following *internal* packages are exported as public API, why?

 org.eclipse.debug.ui.launchview.internal.launcher
 org.eclipse.debug.ui.launchview.internal.services

4) Following *internal* packages are marked as provisional API, why?

 org.eclipse.debug.ui.launchview.internal;x-internal:=true,
 org.eclipse.debug.ui.launchview.internal.impl;x-internal:=true,
 org.eclipse.debug.ui.launchview.internal.model;x-internal:=true,
 org.eclipse.debug.ui.launchview.internal.view;x-internal:=true

5) org.eclipse.debug.ui.launchview.internal.services.LaunchModel is an interface but doesn't follow naming conventions.

6) Two services are published, but as mentioned above, there 

6) Double click on Ant config produces unhandled NPE

Caused by: java.lang.NullPointerException
	at org.eclipse.debug.ui.launchview.internal.impl.DebugCoreLaunchObject.edit(DebugCoreLaunchObject.java:142)
	at org.eclipse.debug.ui.launchview.internal.view.EditAction.run(EditAction.java:47)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:58)
	... 33 more

Can we quickly fix the issues mentioned above, or *at least* quickly remove "public internal API" things?
Comment 43 Eclipse Genie CLA 2021-09-17 10:02:17 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185558
Comment 44 Lars Vogel CLA 2021-09-17 10:03:38 EDT
(In reply to Andrey Loskutov from comment #42)
> Immediate issues I've seen:

> 4) Following *internal* packages are marked as provisional API, why?
> 
>  org.eclipse.debug.ui.launchview.internal;x-internal:=true,
>  org.eclipse.debug.ui.launchview.internal.impl;x-internal:=true,
>  org.eclipse.debug.ui.launchview.internal.model;x-internal:=true,
>  org.eclipse.debug.ui.launchview.internal.view;x-internal:=true

While in the past Eclipse project had to export all packages, this requirement has been removed ~6 months ago, so we do not need to export everything. It is still OK to do so, if needed. 

Andrey, check existing (old) plug-ins which all export their internal packages. 

I pushed a Gerrit to remove that, Markus, let me know if it ok to remove that export.
Comment 45 Lars Vogel CLA 2021-09-17 10:05:24 EDT
(In reply to Andrey Loskutov from comment #42)
> Immediate issues I've seen:
> 
> 1) Double click launches the config. I've expected it would "Edit" config.
> May be that should have a toggle like in the History view (Double click
> edits or starts execution).

I like the double-click -> Run behavior. If you want this to be customizable, please open a new bug.

>) Double click on Ant config produces unhandled NPE

> Caused by: java.lang.NullPointerException

Markus, please have a look.
Comment 46 Andrey Loskutov CLA 2021-09-17 10:12:28 EDT
Also would be good to fix this warning:

The constructor FilteredTree(Composite, int, PatternFilter, boolean) is deprecated	LaunchViewImpl.java	/org.eclipse.debug.ui.launchview/src/org/eclipse/debug/ui/launchview/internal/view	line 93	Java Problem
Comment 47 Lars Vogel CLA 2021-09-17 10:43:43 EDT
(In reply to Andrey Loskutov from comment #46)
> Also would be good to fix this warning:
> 
> The constructor FilteredTree(Composite, int, PatternFilter, boolean) is
> deprecated	LaunchViewImpl.java
> /org.eclipse.debug.ui.launchview/src/org/eclipse/debug/ui/launchview/
> internal/view	line 93	Java Problem

Done with new Gerrit.
Comment 48 Eclipse Genie CLA 2021-09-17 10:44:23 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185560
Comment 49 Andrey Loskutov CLA 2021-09-17 10:51:47 EDT
(In reply to Lars Vogel from comment #44)
> Andrey, check existing (old) plug-ins which all export their internal
> packages. 

I assume you mean plugins that export internal packages for tests only. That plugin has no clients & no API so far, it has nothing to export.
Comment 50 Lars Vogel CLA 2021-09-17 11:01:06 EDT
(In reply to Andrey Loskutov from comment #49)
> (In reply to Lars Vogel from comment #44)
> > Andrey, check existing (old) plug-ins which all export their internal
> > packages. 
> 
> I assume you mean plugins that export internal packages for tests only. That
> plugin has no clients & no API so far, it has nothing to export.

The new rule is that we do not have to (but can) export internal packages anymore. So you are right according to the current rules.
Comment 52 Andrey Loskutov CLA 2021-09-17 11:22:12 EDT
(In reply to Lars Vogel from comment #50)
> The new rule is that we do not have to (but can) export internal packages
> anymore. So you are right according to the current rules.

Sorry, I have no clue about "new rule", can you give some link to that rule?

If a package is exported as "public API" it shouldn't be "internal", or it should specify friends to which it is exported (typically tests, and that means it is not public API in a general sense). These rules are there since ever, and I haven't heard we are going to change that or already changed that.
Comment 54 Markus Duft CLA 2021-09-18 11:54:49 EDT
Well - the exports where there for a reason indeed :) The internal (i know, i know) APIs are used by LcDSL (https://github.com/ssi-schaefer/lcdsl) an extension for the view (and other) which allows a nicer definition of launch configurations. This will no longer work without the exports.

The reason why they are not official API is solely that the public API for the bundle raised some dicusssions which I could not address on the initial contribution (time wise). Therefore I made it internal so that no guarantees on the API are given, but still use it in LcDSL.
Comment 55 Andrey Loskutov CLA 2021-09-18 12:20:44 EDT
(In reply to Markus Duft from comment #54)
> The reason why they are not official API is solely that the public API for
> the bundle raised some dicusssions which I could not address on the initial
> contribution (time wise). Therefore I made it internal so that no guarantees
> on the API are given, but still use it in LcDSL.

That's not how it should be in the platform. Please work on defining the "right" API that could be placed in non internal package.

Also would be great to have at least some smoke tests for the view (but real unit tests are also welcome).

Beside this, other points from my comment 42 must be addressed too, and ideally someone could do a real review of the code (I didn't).
Comment 56 Sarika Sinha CLA 2021-09-20 00:08:32 EDT
(In reply to Andrey Loskutov from comment #55)
> (In reply to Markus Duft from comment #54)
> > The reason why they are not official API is solely that the public API for
> > the bundle raised some dicusssions which I could not address on the initial
> > contribution (time wise). Therefore I made it internal so that no guarantees
> > on the API are given, but still use it in LcDSL.
> 
> That's not how it should be in the platform. Please work on defining the
> "right" API that could be placed in non internal package.
> 
> Also would be great to have at least some smoke tests for the view (but real
> unit tests are also welcome).
> 
> Beside this, other points from my comment 42 must be addressed too, and
> ideally someone could do a real review of the code (I didn't).

@Markus, we definitely need some smoke tests to start with and the API definitions.

@Lars, can you please confirm if you already did the code review.

I haven't done it.
Comment 57 Markus Duft CLA 2021-09-20 01:42:05 EDT
(In reply to Andrey Loskutov from comment #55)
> (In reply to Markus Duft from comment #54)
> > The reason why they are not official API is solely that the public API for
> > the bundle raised some dicusssions which I could not address on the initial
> > contribution (time wise). Therefore I made it internal so that no guarantees
> > on the API are given, but still use it in LcDSL.
> 
> That's not how it should be in the platform. Please work on defining the
> "right" API that could be placed in non internal package.
> 
> Also would be great to have at least some smoke tests for the view (but real
> unit tests are also welcome).
> 
> Beside this, other points from my comment 42 must be addressed too, and
> ideally someone could do a real review of the code (I didn't).

There is some well defined API in the services package. AFAIR it just didn't meet the taste, and/or does not look like other APIs - it is declarative service based. I did not want to hinder merging this change just due to *officially* exposing APIs. If the internal packages are not exported now tho that would totally break things for us. So even if this is no longer guideline (which is a bad decision in my opinion - we had to access internals way to often to get things to work in various places in the past), I would like to request to revert the commit removing the exports until we sorted this out properly and to the liking of everyone.
Comment 58 Andrey Loskutov CLA 2021-09-20 02:00:28 EDT
(In reply to Markus Duft from comment #57)
> (In reply to Andrey Loskutov from comment #55)
> > (In reply to Markus Duft from comment #54)
> > > The reason why they are not official API is solely that the public API for
> > > the bundle raised some dicusssions which I could not address on the initial
> > > contribution (time wise). Therefore I made it internal so that no guarantees
> > > on the API are given, but still use it in LcDSL.
> > 
> > That's not how it should be in the platform. Please work on defining the
> > "right" API that could be placed in non internal package.
> > 
> > Also would be great to have at least some smoke tests for the view (but real
> > unit tests are also welcome).
> > 
> > Beside this, other points from my comment 42 must be addressed too, and
> > ideally someone could do a real review of the code (I didn't).
> 
> There is some well defined API in the services package. AFAIR it just didn't
> meet the taste, and/or does not look like other APIs - it is declarative
> service based. I did not want to hinder merging this change just due to
> *officially* exposing APIs. If the internal packages are not exported now
> tho that would totally break things for us. So even if this is no longer
> guideline (which is a bad decision in my opinion - we had to access
> internals way to often to get things to work in various places in the past),
> I would like to request to revert the commit removing the exports until we
> sorted this out properly and to the liking of everyone.

I wouldn't say anything if that "internal" API would be in a 3rd party code not maintained by Eclipse platform. But now it is merged, and every API we define should be *properly* defined, because it will be impossible to change it later (compatibility is the key). So if there was no agreement made how to define that API and tjat API is necessary to 3rd parties, we can either work on defining proper API towards M3, or we can revert contribution completely until we've done proper work for inclusion it into the platform.
Comment 59 Lars Vogel CLA 2021-09-20 02:54:09 EDT
(In reply to Andrey Loskutov from comment #58)
> (In reply to Markus Duft from comment #57)

Releasing as provitional API should also be fine, as I said this used to be mandatory before January 7, 2020 - McQ, Alex, Dani, Tom, Lars, https://wiki.eclipse.org/Eclipse/PMC or https://bugs.eclipse.org/bugs/show_bug.cgi?id=553709

IMHO bringing in a contribution should not result in getting "punished" by having all internal classes not accessable. If Andrey or Sarika insist in not having to export these internal packages as provisitional API, I can ask the PMC to make a decision.

> @Lars, can you please confirm if you already did the code review.

@Sarika

Yes I reviewed and tested the code. I also refactored it slightly. Of course I'm not an expert of launching API so I would be nice if you could also check.
Comment 60 Andrey Loskutov CLA 2021-09-20 03:00:18 EDT
(In reply to Lars Vogel from comment #59)
> (In reply to Andrey Loskutov from comment #58)
> > (In reply to Markus Duft from comment #57)
> 
> Releasing as provitional API should also be fine, as I said this used to be
> mandatory before January 7, 2020 - McQ, Alex, Dani, Tom, Lars,
> https://wiki.eclipse.org/Eclipse/PMC or
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=553709
> 
> IMHO bringing in a contribution should not result in getting "punished" by
> having all internal classes not accessable. If Andrey or Sarika insist in
> not having to export these internal packages as provisitional API, I can ask
> the PMC to make a decision.

I'm not sure we read same, see what was decided in bug 553709 comment 14:

- Newly added internal packages should not use x-internal exports unless there are very good reasons

- For existing packages the x-internal export can be removed with PMC approval

It does NOT mean newly added internal packages could be exported to everyone as API.
Comment 61 Mickael Istria CLA 2021-09-20 03:10:43 EDT
FWIW, the rule used to be that *all* packages should be exported, and internal one be named internal + marked with x-internal=true. This is what changed back then IIRC.
The rules are now defined in https://wiki.eclipse.org/Export-Package . One possibility for this case would be to use x-friends until API stabilizes. But it should be clear that by doing this, any Eclipse committer is free to move, remove, modify those packages, and is also free to remove the export-package as well. So it's definitely not an API and the chance that this break clients in a future are relatively high without the Platform project being to blame here.
As Andrey mentioned, best would be to identify, desgin and export a minimal API from now on. Usage of Declarative Services is accepted, but it must be properly documented.
Comment 62 Sarika Sinha CLA 2021-09-20 07:41:22 EDT
Looks like "Launch Configurations" view is not added to the Eclipse builds yet.
Can't see the new plug-in in in Build id: I20210919-1800.

Any step missing?
Comment 63 Jörg Kubitz CLA 2021-09-22 08:40:05 EDT
At least i can confirm the project is added when oomphing platform.

Here is my two cent of observations:
* How to add a launch configuration to Favorites? drag&drop does not work and no pin button or context menu for that?
* Almost all actions are enabled for the empty "Favorites" node. Doesnt make sense.
* double-click to run: Its uncommon for edit centric eclipse workbench but in sense of a launch view it makes sense. On the other hand its not consequent: when i run "Java Application" it runs all of them (cool). But when i double click "Java Application" it does not. I would prefer (multi-)select + a run button
* "Remove" should be named "Delete" - my launch config is gone now. rest in peace.
* Sad there is no "new Configuration" action, no "duplicate", ... as in the modal dialog. 
* i love the "Terminate all" . Unfortunately it doe not work if i run the same configuration twice.
* Also when i start a configuration twice the stop button in the console view does not work anymore!

Overall it's a nice alternative for quick launching but feels incomplete. I hope that will change over time.
Comment 64 Lars Vogel CLA 2021-09-22 11:18:06 EDT
(In reply to Sarika Sinha from comment #62)
> Looks like "Launch Configurations" view is not added to the Eclipse builds
> yet.
> Can't see the new plug-in in in Build id: I20210919-1800.
> 
> Any step missing?

Adding it to the feature?
Comment 65 Andrey Loskutov CLA 2021-09-22 11:25:09 EDT
(In reply to Lars Vogel from comment #64)
> (In reply to Sarika Sinha from comment #62)
> > Looks like "Launch Configurations" view is not added to the Eclipse builds
> > yet.
> > Can't see the new plug-in in in Build id: I20210919-1800.
> > 
> > Any step missing?
> 
> Adding it to the feature?

Can we clarify API problem *before* adding the view to the feature? At the end we will have to remove that again together with the view because no one wants current state anyway.
Comment 66 Markus Duft CLA 2021-09-23 07:23:30 EDT
Would it be acceptable to revert the "remove-exports" commit and give us some time to sort this out? All the exports can be x-internal, so no guarantees are made by platform.

Regarding "nobody wants it anway". We have a few hundred developers using it day to day. Of course there may be quite a lot of things that the view could do *additionally*, but that's a different story. Also some of the more advanced power of it comes only together with LcDSL which provides some more extensions to it - using the not-so-API of course.

It has been *so* incredibly hard to contribute to platform (due to my own restricted time, and due to the difficulties of the change being accepted as is) that honestly at this point I no longer care *at all* if it is in or not. Take it or leave it. I need to maintain it anyway for our company and will continue doing so, wether it's in Eclipse or not. It has had some "this should be in Eclipse" comments on the marketplace though, so I assume it *is* useful to at least some. Our company could not manage its (hundreds of) launch configurations without it - at least not without going through a lot of pain.
Comment 67 Lars Vogel CLA 2021-09-23 07:44:26 EDT
(In reply to Markus Duft from comment #66)
> Would it be acceptable to revert the "remove-exports" commit and give us
> some time to sort this out? All the exports can be x-internal, so no
> guarantees are made by platform.

We discussed this in yesterdays PMC call and decided that it is ok to export it an provisional API. Ideally you should identify the packages which are really needed, move them out of the internal namespace so that once we can release them as API you do not have to refactor your extensions.I will revert the removal.


> Regarding "nobody wants it anway". We have a few hundred developers using it
> day to day. Of course there may be quite a lot of things that the view could
> do *additionally*, but that's a different story. Also some of the more
> advanced power of it comes only together with LcDSL which provides some more
> extensions to it - using the not-so-API of course.

I also like it.

> It has been *so* incredibly hard to contribute to platform (due to my own
> restricted time, and due to the difficulties of the change being accepted as
> is) that honestly at this point I no longer care *at all* if it is in or
> not. Take it or leave it. I need to maintain it anyway for our company and
> will continue doing so, wether it's in Eclipse or not. It has had some "this
> should be in Eclipse" comments on the marketplace though, so I assume it
> *is* useful to at least some. Our company could not manage its (hundreds of)
> launch configurations without it - at least not without going through a lot
> of pain.

Sorry for this, lets try to made this better in the future.
Comment 68 Eclipse Genie CLA 2021-09-23 07:47:57 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185741
Comment 69 Lars Vogel CLA 2021-09-23 07:49:31 EDT
Opened Bug 576225 to define the planned API.
Comment 70 Andrey Loskutov CLA 2021-09-23 08:05:44 EDT
(In reply to Markus Duft from comment #66)
> Would it be acceptable to revert the "remove-exports" commit and give us
> some time to sort this out? All the exports can be x-internal, so no
> guarantees are made by platform.

I have nothing against exports that are internal, but that was not in the patch. In the patch we had "public internal API" offered for all - and that is not OK.

> Regarding "nobody wants it anway". 

Correct quote from comment 65: "no one wants current state anyway", based on your own comment 57: "If the internal packages are not exported now tho that would totally break things for us."

I hope you see the difference. The code "as is" on master is not OK, and this is only what I've meant. I've said nothing about original bundle usefulness - as long as that behaves well I have nothing against it.

> It has been *so* incredibly hard to contribute to platform (due to my own
> restricted time, and due to the difficulties of the change being accepted as
> is) that honestly at this point I no longer care *at all* if it is in or
> not. Take it or leave it. 

If we "take it", it has to follow platform rules, especially if that provides API. Eclipse strongest point for consumers is API promise - if API is published, we will not break it, or only with a very long time (3 years?) allowing others to adopt the code.

It makes no sense to pick some 3rd party plugins to the platform if no one has time to make the contribution "platform ready". It would probably save time for all of us if the plugin would be kept on marketplace.
Comment 72 Eclipse Genie CLA 2021-09-24 05:34:29 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185802
Comment 73 Eclipse Genie CLA 2021-09-24 06:19:19 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185803
Comment 74 Markus Duft CLA 2021-09-24 06:28:51 EDT
I addressed various of the complaints/suggestions/requirements in a series of changes I invited you all to review. I hope this is sufficiently worked out now to be acceptable to all. If there are any remaining open issues please tell me.

I'm aware that the initial test for the view is not sufficient ;) But I have a severe lack of knowledge on how to write *actual* UI tests in Eclipse, maybe somebody has hints for me, or can step up to help me with that a little bit.
Comment 75 Jörg Kubitz CLA 2021-09-24 06:33:03 EDT
(In reply to Markus Duft from comment #74)
> maybe somebody has hints for me, or can step up to help me with that a
> little bit.

EGit has a bunch of Tests which you could look at. For example org.eclipse.egit.ui.view.repositories.GitRepositoriesViewTest
Comment 77 Sarika Sinha CLA 2021-09-30 10:10:49 EDT
I get this error on Mac:
A conflict occurred for ALT+COMMAND+L:
Binding(ALT+COMMAND+L,
	ParameterizedCommand(Command(org.eclipse.debug.ui.launchview.LaunchConfigQuicklaunch,Quick Launch,
		,
		Category(org.eclipse.core.commands.categories.autogenerated,Uncategorized,Commands that were either auto-generated or have no category,true),
		org.eclipse.ui.internal.WorkbenchHandlerServiceHandler@14656be5,
		,,true),null),
	org.eclipse.ui.defaultAcceleratorConfiguration,
	org.eclipse.ui.contexts.window,,,system)
Binding(ALT+COMMAND+L,
	ParameterizedCommand(Command(org.eclipse.jdt.ui.edit.text.java.extract.local.variable,Extract Local Variable,
		Extracts an expression into a new local variable and uses the new local variable,
		Category(org.eclipse.jdt.ui.category.refactoring,Refactor - Java,Java Refactoring Actions,true),
		org.eclipse.ui.internal.WorkbenchHandlerServiceHandler@20be890d,
		,,true),null),
	org.eclipse.ui.defaultAcceleratorConfiguration,
	org.eclipse.ui.contexts.window,,cocoa,system)
Comment 78 Markus Duft CLA 2021-10-01 01:54:45 EDT
(In reply to Sarika Sinha from comment #77)
> I get this error on Mac:
> A conflict occurred for ALT+COMMAND+L:
> Binding(ALT+COMMAND+L,
> 	ParameterizedCommand(Command(org.eclipse.debug.ui.launchview.
> LaunchConfigQuicklaunch,Quick Launch,

That is with https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185800? The shortcut was introduced with this change, in the latest patchset it is not even in there anymore. I wonder where you got this from...?
Comment 79 Sarika Sinha CLA 2021-10-01 02:22:54 EDT
(In reply to Markus Duft from comment #78)
> (In reply to Sarika Sinha from comment #77)
> > I get this error on Mac:
> > A conflict occurred for ALT+COMMAND+L:
> > Binding(ALT+COMMAND+L,
> > 	ParameterizedCommand(Command(org.eclipse.debug.ui.launchview.
> > LaunchConfigQuicklaunch,Quick Launch,
> 
> That is with
> https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185800? The
> shortcut was introduced with this change, in the latest patchset it is not
> even in there anymore. I wonder where you got this from...?

I did not take this gerrit but I did have https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185799
in my workspace.
Comment 80 Markus Duft CLA 2021-10-01 03:47:20 EDT
(In reply to Sarika Sinha from comment #79)
> > 
> > That is with
> > https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185800? The
> > shortcut was introduced with this change, in the latest patchset it is not
> > even in there anymore. I wonder where you got this from...?

This change adds a shortcut...

> 
> I did not take this gerrit but I did have
> https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185799
> in my workspace.

... and this one does not. I cannot explain to you how this change alone would cause this. It does - in fact - not add a single extension...
Comment 82 Sarika Sinha CLA 2021-10-18 13:45:24 EDT
@Markus,
Can you add the N&N?
Comment 83 Eclipse Genie CLA 2021-10-19 01:32:13 EDT
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/186634
Comment 84 Markus Duft CLA 2021-10-19 01:38:12 EDT
Hope I found the correct location :) If not, let me know.
Comment 86 Sarika Sinha CLA 2021-10-19 07:22:55 EDT
(In reply to Markus Duft from comment #84)
> Hope I found the correct location :) If not, let me know.

Thanks Markus for all the effort.
Comment 87 Sarika Sinha CLA 2021-11-10 09:12:48 EST
I20211109-1800