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

Migrate the e4 tools to PDE #1081

Merged
merged 1,294 commits into from
Jan 28, 2024
Merged

Migrate the e4 tools to PDE #1081

merged 1,294 commits into from
Jan 28, 2024

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Jan 28, 2024

Please carefully review if this is the desired state, I'll now start to integrate this into the build ...

Fix #898

Jens Lidestrom and others added 30 commits July 3, 2018 17:25
The following commit introduces type parameters on databinding
classes. This commit cleans up warnings resulting from that.

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2220dc6057030ca7d761b4ed6b7debb50eec3452

Change-Id: Ibf73d868e3a0db750014329d592db2a97c840760
Signed-off-by: Jens Lidestrom <[email protected]>
The Abstract new class Page now persist the last selected package.
(The solution works also for NewAddonCW, NewDynamicMenuContributionCW,
NewImperativeExplressionCW, NewPartCW, and NewToolControlCW)

Change-Id: I4e3ac01c189ec3fa2441f47c7794bf9d9f451f09
Signed-off-by: Patrik Suzzi <[email protected]>
Change-Id: Iec558d1de9f5c5cf1bd9467ea8350148816e0271
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: Id8f44280053188895ba510563c647374dc46327e
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: Ic8f79f160be52f267fcbc1717d60ff8099be90b6
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: I23cab3d2fb96c25491a43e86d05a03bcda2fcf11
Signed-off-by: Lars Vogel <[email protected]>
/org.eclipse.e4.tools.emf.ui/src/org/eclipse/e4/tools/emf/ui/internal/Messages.properties

Change-Id: Ia62912136c42417e10f258b2326a076dc440b0ac
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: I4f4aedbc67eb1210c3b92210739e0326c9908b53
Signed-off-by: Sravan Kumar Lakkimsetti <[email protected]>
editor

Change-Id: I26018408a1f22a00a7b06e2e4561c90e4375da28
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: Iac2dafc26f4cec4759d7b47c4a13a5e1a5255ebc
Signed-off-by: Lars Vogel <[email protected]>
working

Change-Id: I897c7f27c66de4e430698d21bb91f1da4e2cab40
Signed-off-by: Burcu Karlidag <[email protected]>
working

Steps to reproduce:
Create a new Plug-in Project as Rich Client Application
select Eclipse 4 RCP application.

The class URI Button in the Part Descriptor of the created
Application.e4xmi was not working



Change-Id: I498e01bb60e2a023dcbb71bbdae4e9ec67fa9398
Signed-off-by: Burcu Karlidag <[email protected]>
Change-Id: Ie0fc86833c346cdde3648d6c1e3cf92afc4be1e4
Signed-off-by: Christof Joswig <[email protected]>
The Feature Name field is empty.
Pressing the Add button in the Table Viewer leads to a NPE exception.

java.lang.NullPointerException
	at org.eclipse.e4.tools.emf.ui.internal.common.component.StringModelFragment$3.addPressed(StringModelFragment.java:422)

Change-Id: I91ef4e45b654ad8c4d091abbcd2dd904b94da362
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: I7ff26d1ef258f541176773b10ca6c1ebc69403a2
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: I18be37d47d978be4ac5c1283b135dd2ee2b98e8a
Signed-off-by: Christof Joswig <[email protected]>
- Disable down/up bottons for multip selections
- Disable down/up button when last/first element is selected

Fixed issue on windows:
- Keep focus when pressing up/down buttons

Change-Id: Ic4b899a51df19c018abfdfd5df0dd883ad32aab2
Signed-off-by: Christof Joswig <[email protected]>
Goto:
->Model Fragment->Handler->Persisted State table doesn't fill

Change-Id: I07f5baf8d855788d956422b69b4589c35b7ff0e2
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: If33fd165ada604485fc862170dfa9489323f94fb
Signed-off-by: [email protected] <[email protected]>
Change-Id: Ia70b63538cec2b725d1163bd9769a06fdba9a121
Signed-off-by: [email protected] <[email protected]>
Change-Id: Id9ef473a54c42b5fac70e550743d23451186536e
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: I53468f930e539e46740d16d2d600ca3a89c92a85
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: I7d4500bfe8b1c576131964c6df4932e366b99557
Signed-off-by: Christof Joswig <[email protected]>
Change-Id: If279b3b8d5c109bc036a13e4281c701702899617
Signed-off-by: [email protected] <[email protected]>
Change-Id: I584cbf5c2ae5bb215a520bd65d1a7b6232d90b9a
Signed-off-by: [email protected] <[email protected]>
-> align find buttons in E4 Editor

Change-Id: I913808af3fea831deaad44246f3434063070f4c7
Signed-off-by: [email protected] <[email protected]>
Change-Id: Ic441b1f61c8f79c4ab88254bc2d440acd1e12db5
Signed-off-by: [email protected] <[email protected]>
Change-Id: I8b8a7f3548273a3535af1cf63ad3f2c1c7619453
Signed-off-by: Lars Vogel <[email protected]>
classes

Change-Id: Ibd4496bb5180f825b6aca4f17e439bcdffc497a2
Signed-off-by: Lars Vogel <[email protected]>
Change-Id: I2a54dcdb5503d46621908049691848252e6dc66b
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.
In general this looks good, but I suggest to move the e4-tools projects into a folder named e4-tools not just tools.
We already have apitools, so we should be specific here too.

Furthermore the Oomph setup has to be adjusted, but that can also be done subsequently.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 28, 2024

I suggest to move the e4-tools projects into a folder named e4-tools not just tools.
We already have apitools, so we should be specific here too.

I actually don't want to complicate things too much, the files where already moved to ui then moved there and so on.

If we have merged this, we can of course think about moving thing again around but for now I think we should complete this asap to not run into merge conflicts.

@iloveeclipse
Copy link
Member

If we have merged this, we can of course think about moving thing again

I would prefer we would move it only once

around but for now I think we should complete this asap to not run into merge conflicts.

There is no rush, as long as few active committers can have a consensus how a merge should happen.

@HannesWell
Copy link
Member

If we have merged this, we can of course think about moving thing again

I would prefer we would move it only once

Me too.
Does everyone participating in this PR currently is fine with e4-tools?

@laeubi if you have to now time to move it I can adjust this PR for you quickly.

@iloveeclipse
Copy link
Member

And yes, looking on the new structure I agree with Hannes, "tools" is non saying, while "e4-tools" is much better.
To be consistent with "apitools" may be better to have "e4tools" (no dash)?
Beside this, history seem to be OK after merge.

@HannesWell
Copy link
Member

To be consistent with "apitools" may be better to have "e4tools" (no dash)?

Thought about this as well and personally would have liked it more with a dash. But since renaming apitools just for that is not really worth the effort I'm also fine with e4tools to have it consistent.

@merks
Copy link
Contributor

merks commented Jan 28, 2024

I’m fine with or without the dash.

@HannesWell
Copy link
Member

HannesWell commented Jan 28, 2024

@laeubi I have it almost ready locally, with an intermediate commit just after the merge to move it to e4tools (without dash).
If you are fine, I can force push it to this branch.

@HannesWell
Copy link
Member

In order to speed this up, I took the liberty to update this PR with the suggested move after the merge and adapted to following commits accordingly.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 28, 2024

I would prefer we would move it only once

That's the point it is NOT moved see:

https://github.com/eclipse-platform/eclipse.platform.ui/tree/master/tools

I just imported all commits from the repository as is so don't want to add additional "noise" for the inital import.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 28, 2024

So can we PLEASE delay any "make it look nicer" to follow up PRs?

Merging repositories is already a daunting task so adding addition "cleanup" as part of the merge is not really anything beneficial beside it is required for the build to succeed.

@HannesWell
Copy link
Member

I just imported all commits from the repository as is so don't want to add additional "noise" for the inital import.

Importing these commits is already 'noise' and moving them around is another 'noise'. Therefore I would have done that (closely) together in one 'noise'.

Merging repositories is already a daunting task so adding addition "cleanup" as part of the merge is not really anything beneficial beside it is required for the build to succeed.

Well that's why I tried to help you with that extra work, but if you think it is so much better to do that separately and adjust the build twice you can restore your previous state and it can be moved later.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 28, 2024

but if you think it is so much better to do that separately and adjust the build twice

I won't do it at all for a simple reason

  1. we previously always have choose the name of the original repo that was "tools"
  2. we don't have e3tools either so e4tools is missleading, also the ui folder already contains some "e4 tools" so this makes it not clearer or better
  3. we now have two times interrupting the history, e.g. look at this file it show:

Renamed from tools/bundles/org.eclipse.e4.tools.compat/src/org/eclipse/e4/tools/compat/internal/CopyAction.java
(Browse History)

if you then follow it it shows you the previous move from Lars:
Renamed from bundles/org.eclipse.e4.tools.compat/src/org/eclipse/e4/tools/compat/internal/CopyAction.java
(Browse History)

and now you see the "real" history, so for me renaming the folder does not bring anything useful in terms of git history either...

@HannesWell
Copy link
Member

  • we previously always have choose the name of the original repo that was "tools"

But that was for another repository, which does not have other 'tools'.

  • we don't have e3tools either so e4tools is missleading, also the ui folder already contains some "e4 tools" so this makes it not clearer or better

And we don't have e2 and e1, so I don't understand why it is a prerequisite?
In general I agree that the overall structure is not ideal, but has probably grown historically, but I don't see a big need to change the existing structure (which affects existing PRs and local changes). In the end the path within the repo does not really matter.
But if we add new files to the repo, we can and should try how to fit that best into the existing structure (existing PRs local changes have to be adapted any way). And since all the added projects are named org.eclipse.e4.tools* and are therefore different from the existing projects, they should IMHO reside in a specific location.

  • we now have two times interrupting the history, e.g. look at this file it show:

That's right, but I don't see why this is a problem as long as within git-blame per line is preserved (which a move does).

@HannesWell
Copy link
Member

But as we have discussed privately in the end the location is not a show stopper and all of use can do more important things.
@iloveeclipse and @merks if you are fine with the current location in e4tools I'll continue with the merge to not delay this further.

@HannesWell HannesWell requested a review from merks January 28, 2024 12:57
@iloveeclipse
Copy link
Member

if you are fine with the current location in e4tools I'll continue with the merge to not delay this further.

Yes, I'm fine.

@HannesWell HannesWell merged commit 8a229fc into eclipse-pde:master Jan 28, 2024
11 of 17 checks passed
@HannesWell
Copy link
Member

Merged it now. Thank you @laeubi for this work.

@akurtakov
Copy link
Member

Thanks for all the work guys! I try to take weekends off computers so you're most likely gonna see delays during that time in my responses.

@mickaelistria
Copy link
Contributor

Thank you very much for this refactoring!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate the e4 tools to PDE