Skip to content

Revert highlighting fix #1957

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

Merged

Conversation

fedejeanne
Copy link
Contributor

@iloveeclipse
Copy link
Member

You cannot revert bundle versions for published bundles!

@iloveeclipse
Copy link
Member

We have IBuild there with these versions.
The problem is that all new builds seeing IBuild versions will always pick the highest (broken) version of bundles.

@merks : WDYT, should we mark it unstable and revert versions, or better don't try to revert versions?

@akurtakov
Copy link
Member

Please don't revert versions, everyone that already updated to latest I-build (I am aware and will redownload myself but others might not be) will not get updates.

@fedejeanne fedejeanne force-pushed the revert_highlighting_fix branch from 0b0f492 to 22dcffa Compare June 12, 2024 07:13
@fedejeanne
Copy link
Contributor Author

fedejeanne commented Jun 12, 2024

Please don't revert versions, everyone that already updated to latest I-build (I am aware and will redownload myself but others might not be) will not get updates.

👍
Done in 22dcffa

@merks
Copy link
Contributor

merks commented Jun 12, 2024

Yes, please don't revert versions. These things have already escape and I are my pool. Also, I think we can assume this will eventually be fixed properly and hence all the bundle will be incremented at some point in the not-to-distant future.

@fedejeanne
Copy link
Contributor Author

Yes, please don't revert versions.

@merks versions should be as published now --> 22dcffa

I think we can assume this will eventually be fixed properly and hence all the bundle will be incremented at some point in the not-to-distant future.

I'd say it should be possible to wait until later today to apply this PR and give @BeckerWdf / @Christopher-Hermann some time to find a better fix.

@iloveeclipse would that be ok for you? Since you are in Linux, you are seeing the problems in your IDE already. Colors can be changed though (see #1690 (comment)), so it should be endurable for a couple of hours (?)

@fedejeanne
Copy link
Contributor Author

Please don't revert versions, everyone that already updated to latest I-build (I am aware and will redownload myself but others might not be) will not get updates.

Hm, on a second thought, this means that not only should I not revert (decrease) versions but I should actually increase versions, right @akurtakov ? I mean those who already updated their IDE and have #1690 in it should be able to get an update with #1957 (this PR) tomorrow.

I'll increase versions in a separate commit in a minute.

@merks
Copy link
Contributor

merks commented Jun 12, 2024

Yes, you should keep the versions as they are. They should end up with a new qualifier in a new build via the other revert commit in the projects.

@mickaelistria
Copy link
Contributor

this means that not only should I not revert (decrease) versions but I should actually increase versions

No need to modify the x.y.z, the version qualifier will be increased at build-time because as it will be updated with the commit merge date, which would be newer.

@merks
Copy link
Contributor

merks commented Jun 12, 2024

I'm okay with not doing a revert at all if this can be addressed in the next few hours. Yesterday when was testing the removal of a feature, the build itself took like 3.5 hours and then the testing seemed to take another 4, so there is along delay between when you might commit a revert (without reverting versions) and a build from that being available.

@merks
Copy link
Contributor

merks commented Jun 12, 2024

Please don't revert versions, everyone that already updated to latest I-build (I am aware and will redownload myself but others might not be) will not get updates.

Hm, on a second thought, this means that not only should I not revert (decrease) versions but I should actually increase versions, right @akurtakov ? I mean those who already updated their IDE and have #1690 in it should be able to get an update with #1957 (this PR) tomorrow.

I'll increase versions in a separate commit in a minute.

Just in case the rapid responses are not 100% clear, the idea is to revert everything except the version changes and as @mickaelistria correctly points out, this will result in new qualifier versions so that we can update to the reverted versions.

Hopefully the problem is just fixed very quickly...


I'm out for a few hours, otherwise I could test that the installer works with whatever fixes are being applied...

@fedejeanne fedejeanne force-pushed the revert_highlighting_fix branch from 22dcffa to 537394a Compare June 12, 2024 07:43
Copy link
Contributor

Test Results

 1 510 files  +  300   1 510 suites  +300   1h 11m 26s ⏱️ + 19m 5s
 7 659 tests  -     5   7 430 ✅  -     6  229 💤 + 1  0 ❌ ±0 
22 298 runs  +6 196  21 750 ✅ +6 117  548 💤 +79  0 ❌ ±0 

Results for commit 537394a. ± Comparison against base commit 521543f.

This pull request removes 5 tests.
org.eclipse.jface.internal.text.TableOwnerDrawSupportTest ‑ testPaintSelectionFocus
org.eclipse.jface.internal.text.TableOwnerDrawSupportTest ‑ testPaintSelectionNoFocus
org.eclipse.jface.internal.text.contentassist.CompletionTableDrawSupportTest ‑ testInstall
org.eclipse.jface.internal.text.contentassist.CompletionTableDrawSupportTest ‑ testPaintNonFocusSelectionInFocusColors
org.eclipse.jface.internal.text.contentassist.CompletionTableDrawSupportTest ‑ testStoreStyleRanges
This pull request skips 1 test.
UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay

@iloveeclipse
Copy link
Member

On Linux (and probably on other OSes) we have more problems that needs a fix: #1956 (wrong selection color, missing selection icon, missing foreground coloring on trees).

@Christopher-Hermann : if you could fix all of them today till evening, I can validate your fixes.
Otherwise I would like to merge this revert for the next IBuild this night.

@Christopher-Hermann
Copy link
Contributor

@Christopher-Hermann : if you could fix all of them today till evening, I can validate your fixes. Otherwise I would like to merge this revert for the next IBuild this night.

Let's merge this revert. I will not mange to fix the issues by today.

@akurtakov
Copy link
Member

FWIW, I strongly recommend testing colors issues/changes with both theming engine enabled and disabled, IMO it does a lot more than needed trying to workaround issues on lower lever thus breaking UI when an issue underneath is fixed.

@iloveeclipse
Copy link
Member

I strongly recommend testing colors issues/changes with both theming engine enabled and disabled

Exact. This is what I did on #1956 and the problem appears in both modes.

@iloveeclipse
Copy link
Member

I've quickly tested this PR and it fixes all the issues I've reported in #1956. So let's merge.

@iloveeclipse iloveeclipse merged commit c06713f into eclipse-platform:master Jun 12, 2024
15 of 16 checks passed
@iloveeclipse
Copy link
Member

Should I trigger another IBuild now to see the real test results without JFace related errors?

@merks
Copy link
Contributor

merks commented Jun 12, 2024

Yes please.

@iloveeclipse
Copy link
Member

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.

6 participants