Community
Participate
Working Groups
Only the keyboard shortcut Ctrl+R stopped working for me a while ago. Pressing Ctrl+R while debugging works in 4.12 does nothing in 4.13M1 All other variants of Run to line (top menu, context menu, click on annotation ruler) work as usual.
Can reproduce this. Will have to track when did it stop working. Don't remember any change in this area.
Traced down to stopped working in between Build id: I20190620-0130 and Build id: I20190620-1800 There was no commit from jdt.debug and platform debug commit was cleanup which I tried reverting but does not fix the issue. Looks like it is broken by some platform commit. @Andrey, Do you have any idea about any commit which can break this?
This is regression from https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c896914047a6e8ddd92b48bb66e2a682417fda17 , see https://git.eclipse.org/r/#/c/144099/ I haven't check what exact and how was broken, but it is another confirmation for me that mass code changes are not fun at all and should always go with a bugzilla and a *real* good reason for a change. I will check what exact was broken after an hour or so.
(In reply to Andrey Loskutov from comment #3) > This is regression from > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=c896914047a6e8ddd92b48bb66e2a682417fda17 > , see https://git.eclipse.org/r/#/c/144099/ How did you determine it? A local revert of the commit? > I haven't check what exact and how was broken, but it is another > confirmation for me that mass code changes are not fun at all Indeed, they're not. > and should always go with a bugzilla How would a bugzilla ticket have helped here. > and a *real* good reason for a change. Code readability and simplicity are real good reasons for this change, especially for projects that have as constant goal of being open to new contributors. The absence of good tests to catch regressions is IMO where rants should be directed.
New Gerrit change created: https://git.eclipse.org/r/146157
(In reply to comment #4) > The absence of good tests to catch regressions is IMO where rants should be > directed. Exactly!!!
@Andrey, Did you verify that the revert fixes the issue? Unfortunately I don't have the setup to verify. Can I release the revert ?
(In reply to Sarika Sinha from comment #7) > @Andrey, > Did you verify that the revert fixes the issue? > Unfortunately I don't have the setup to verify. Can I release the revert ? I will push a smaller change in few minutes. (In reply to Andrey Loskutov from comment #3) > This is regression from > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=c896914047a6e8ddd92b48bb66e2a682417fda17 > , see https://git.eclipse.org/r/#/c/144099/ > > I haven't check what exact and how was broken, but it is another > confirmation for me that mass code changes are not fun at all and should > always go with a bugzilla and a *real* good reason for a change. > > I will check what exact was broken after an hour or so. The problem with the patch is that it does NOT a "safe" 1:1 code replacement. List.remove(Object) is NOT 1:1 replacement of List.removeAll(Collection)!!! Whoever invented this "clean up" should go and revert all cases where it was applied on List objects! A list is not a set, and can contain more than one "equal" element. So removing "a" from list {"a", "a"} via remove(Object) gives you {"a"} list, not an empty list that would be after removeAll(Collection). Exact this happens with https://git.eclipse.org/r/#/c/144099/9/bundles/org.eclipse.e4.ui.services/src/org/eclipse/e4/ui/internal/services/ContextContextService.java@124 (In reply to Mickael Istria - away until July 15th from comment #4) > (In reply to Andrey Loskutov from comment #3) > > This is regression from > > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=c896914047a6e8ddd92b48bb66e2a682417fda17 > > , see https://git.eclipse.org/r/#/c/144099/ > > How did you determine it? A local revert of the commit? Yes, thanks to Sarika it was clear which commit to revert. > > I haven't check what exact and how was broken, but it is another > > confirmation for me that mass code changes are not fun at all > > Indeed, they're not. > > > and should always go with a bugzilla > > How would a bugzilla ticket have helped here. We would be able to trace it to the concrete ticket, release, link to this bug, etc. Assuming this would slip into 4.13, we could point to this ticket, know in which release it was introduced etc. > > and a *real* good reason for a change. > > Code readability and simplicity are real good reasons for this change, > especially for projects that have as constant goal of being open to new > contributors. The problem is that in many cases that changes aren't really justified, carefully reviewed, nor tested, and I NEVER saw any new test added by such changes. I don't also buy the argument with new contributors - the change is coming from a committer. > The absence of good tests to catch regressions is IMO where rants should be > directed. Yes, the absence of test is a perfect reason to spent time to write those. What about that next time a big "cleanup" wave starts, BEFORE the cleanup, first run coverage tests, identify weak (not covered by tests) places in the software and add more tests in the cleanup area? Only after that we can apply "clean up" safely. This is how good, "agile" developer would do - tests first. This will also attract new contributors, everyone like tests. This would also prevent this bug. Anyway, I will check the patch, remove all removeAll() places on Lists and push it soon.
New Gerrit change created: https://git.eclipse.org/r/146171
Gerrit change https://git.eclipse.org/r/146171 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e641340ef2ea13accd8c3b65a4b1cedd25280a1d
(In reply to Eclipse Genie from comment #10) > Gerrit change https://git.eclipse.org/r/146171 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e641340ef2ea13accd8c3b65a4b1cedd25280a1d > Thanks Andrey!
Eclipse SDK Version: 2019-09 (4.13) Build id: I20190716-1800
Before closing this bug, shouldn't there be a test added or at least a comment in the code that warns against using removeAll there? (I know, the comment probably wouldn't have helped this time as seen in bug 548727, but still..)
(In reply to Till Brychcy from comment #13) > Before closing this bug, shouldn't there be a test added or at least a > comment in the code that warns against using removeAll there? > > (I know, the comment probably wouldn't have helped this time as seen in bug > 548727, but still..) Unfortunately I'm pretty busy, the free time I have I usually spend for debugging regressions and hunting behind new test fails. Regarding a comment: the code uses lists. It is clear the lists *can* contain multiple equal elements, so a comment would probably state an obvious fact. I would propose to investigate why the lists were used first (and not LinkedHashSet's for example), and if it was really the intent of original authors to keep multiple equal elements in the lists (or if the bad refactoring uncovered bad code). I hope someone from the "clean up" front could take this over? Assuming the next clean up wave is coming, this would be a good example. => all this please in a new bug.
(In reply to Till Brychcy from comment #13) > > (I know, the comment probably wouldn't have helped this time as seen in bug > 548727, but still..) If this was already reverted in equinox for not being equivalent, I am surprised that the owner did not consider to revisit https://git.eclipse.org/r/#/c/144099/ and act accordingly.
(In reply to Andrey Loskutov from comment #14) > (In reply to Till Brychcy from comment #13) > > Unfortunately I'm pretty busy, the free time I have I usually spend for I didn't mean that *you* have to do this. Still I don't think the issue is not solved in the current state. > > Regarding a comment: the code uses lists. It is clear the lists *can* > contain multiple equal elements, so a comment would probably state an > obvious fact. I think if it was obvious, that lists are used at that place and that only one of multiple equals elements should be removed, the bug would not have been introduced - at least a comment should point that out. > > I would propose to investigate why the lists were used first (and not > LinkedHashSet's for example), and if it was really the intent of original > authors to keep multiple equal elements in the lists (or if the bad > refactoring uncovered bad code). That would be even better, but won't be achieved by simply closing the topic. > > I hope someone from the "clean up" front could take this over? Assuming the +1 as this work belongs to the clean up. > > => all this please in a new bug. You can create one, but why? There is plenty of time till 4.13M3. I'd suggest to simply reopen this and unassign yourself - or maybe assign Lars?
(In reply to Till Brychcy from comment #16) > You can create one, but why? There is plenty of time till 4.13M3. > I'd suggest to simply reopen this and unassign yourself - or maybe assign > Lars? This is a regression, and it is fixed now. Reopening this bug will be confusing => therefore if someone can/will continue on points listed above, please do it on a new bug.
Misc information: The invalid refactoring from `remove` to `removeAll` was most probably caused by a bug in the AutoRefactor tool. I reported here: https://github.com/JnRouvignac/AutoRefactor/issues/400
