Community
Participate
Working Groups
Created attachment 274593 [details] screenshot The minimap rectangle selection does not show the same content as the editor and is off by several lines. See the attached screenshot. This is because both the minimap rectangle height as well as the font size are calculated by multiplying the original editor height and font size by 0.2. But the text with 0.2 times font size does NOT automatically have exactly 0.2 times the height. Therefore the better way for calculating the height of the minimap rectangle would be: * get the number of visible lines N in the editor * calculate the height of N lines of text in the minimap text widget via SWT methods, font metrics or similar
New Gerrit change created: https://git.eclipse.org/r/125410
Created attachment 274814 [details] Minimap bug Here a demo which shows you the bug when your editor has few lines.
Created attachment 275138 [details] Issue with patchset 3 on mac
Created attachment 275139 [details] iss#
Comment on attachment 275139 [details] iss# issue with patchset 3 on mac and word wrap active
Created attachment 275140 [details] my test file to reproduce issues reported for patchset 3
I have the same issue with Windows OS.
I have the same issue with Windows OS with or without word wrap.
I think this issue really should be fixed before we ship it with 2018-09 to our customers. In this state the feature really doesn't make a good impression.
New Gerrit change created: https://git.eclipse.org/r/126950
I have commited an alternative implementation for the calculation of the height. I use the same approach to calculate the end of the window as it was used already for the start as this seems to be accurate in all circumstances. Have tested it under windows 10. Works well with and without word wrap enabled.
Thanks @Arne! I will test your gerrit patch. IMHO I think we should add too a test for this issue to complete tests that I have written at https://git.eclipse.org/r/#/c/126948/ (need a review)
(In reply to Arne Deutsch from comment #11) > I have commited an alternative implementation for the calculation of the > height. I use the same approach to calculate the end of the window as it was > used already for the start as this seems to be accurate in all > circumstances. Have tested it under windows 10. Works well with and without > word wrap enabled. I've tried it. I see two issues: - On the Mac, the last line seems to be not included (Possible solution: See gerrit) - If the text is shorter than the displayed area, the box is too small (and also not updated, if lines are added)
(In reply to Angelo ZERR from comment #12) > Thanks @Arne! I will test your gerrit patch. IMHO I think we should add too > a test for this issue to complete tests that I have written at > https://git.eclipse.org/r/#/c/126948/ (need a review) +1 for a test. I also left some comments in gerrit.
I have fixed the behaviour according to your comments. But I fear the adding tests would require a major refactoring beforehand. Right now all calculations and the result are hidden inside the render logic. To write tests against this one would need to seperate the calculations from the render logic into a seperate class.
> To write tests against this one would need to seperate the calculations from the render logic into a seperate class. Please do that!
(In reply to Angelo ZERR from comment #16) > > To write tests against this one would need to seperate the calculations from the render logic into a seperate class. > > Please do that! should we separate this into a new commit or do you want to do it in the current commit?
> should we separate this into a new commit or do you want to do it in the current commit? I think in the same commit is better.
I have started to refactor MinimapWidget but it is really a lot of work. I can not invest that much time to make the code clean and testable. I would suggest to take the fix as it is now. If you relly want to refactor the class to make it testable and add tests I would suggest to create a different ticket for that.
(In reply to Arne Deutsch from comment #19) > I have started to refactor MinimapWidget but it is really a lot of work. I > can not invest that much time to make the code clean and testable. I would > suggest to take the fix as it is now. If you relly want to refactor the > class to make it testable and add tests I would suggest to create a > different ticket for that. Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse?
Yes, but can not reproduce it at my machine. Appart from this I think the original bug has been already fixed. Together with some others that we have found during the process. I think it makes sense to create a new bug ticket for that specific issue. Would you do that?
(In reply to Arne Deutsch from comment #21) > Yes, but can not reproduce it at my machine. Appart from this I think the > original bug has been already fixed. Together with some others that we have > found during the process. I think it makes sense to create a new bug ticket > for that specific issue. Would you do that? This issue doesn’t appear without your patch so i‘d say it belongs to this bug. Can someone with a Mac please try to reproduce this?
(In reply to Till Brychcy from comment #22) > (In reply to Arne Deutsch from comment #21) > > Yes, but can not reproduce it at my machine. Appart from this I think the > > original bug has been already fixed. Together with some others that we have > > found during the process. I think it makes sense to create a new bug ticket > > for that specific issue. Would you do that? > This issue doesn’t appear without your patch so i‘d say it belongs to this > bug. > Can someone with a Mac please try to reproduce this? I tried. I could not. But maybe I did not get your description correctly. I started a target IDE from my DevIDE and restarted this. Box height looked correct. Can you explain your steps in more details or do a screen recording?
(In reply to Matthias Becker from comment #23) > (In reply to Till Brychcy from comment #22) > > (In reply to Arne Deutsch from comment #21) > > > Yes, but can not reproduce it at my machine. Appart from this I think the > > > original bug has been already fixed. Together with some others that we have > > > found during the process. I think it makes sense to create a new bug ticket > > > for that specific issue. Would you do that? > > This issue doesn’t appear without your patch so i‘d say it belongs to this > > bug. > > Can someone with a Mac please try to reproduce this? > > I tried. I could not. But maybe I did not get your description correctly. I > started a target IDE from my DevIDE and restarted this. Box height looked > correct. > Can you explain your steps in more details or do a screen recording? I tried a bit and with some editor configurations it doesn't happen. I general I have multiple .java files open (some of them short) plus a .classpath plus a pom.xml I'll try to find to make it reproducible over the weekend.
I have tried to make it reproducible click-by-click starting from a fresh workspace, but failed. But it happens more often, if I start in a workspace that has a lot of editors open. I don't know if you should just commit the change as is and wait if somebody figures out how to reproduce this.
(In reply to Till Brychcy from comment #25) > I don't know if you should just commit the change as is and wait if somebody > figures out how to reproduce this. The question is: Is the state with this change better then the current state in master? I would say "yes" because the mini map currently is wrong for (almost) all cases. What do you think?
(In reply to Matthias Becker from comment #26) > (In reply to Till Brychcy from comment #25) > > I don't know if you should just commit the change as is and wait if somebody > > figures out how to reproduce this. > > The question is: Is the state with this change better then the current state > in master? > > I would say "yes" because the mini map currently is wrong for (almost) all > cases. > What do you think? Yes, even if it more obviously broken if the restart-problem does appear.
(In reply to Till Brychcy from comment #27) > (In reply to Matthias Becker from comment #26) > > (In reply to Till Brychcy from comment #25) > > > I don't know if you should just commit the change as is and wait if somebody > > > figures out how to reproduce this. > > > > The question is: Is the state with this change better then the current state > > in master? > > > > I would say "yes" because the mini map currently is wrong for (almost) all > > cases. > > What do you think? > > Yes, even if it more obviously broken if the restart-problem does appear. So what do we do? Merge this fix or not?
(In reply to Matthias Becker from comment #28) > (In reply to Till Brychcy from comment #27) > > (In reply to Matthias Becker from comment #26) > > > (In reply to Till Brychcy from comment #25) > > > > I don't know if you should just commit the change as is and wait if somebody > > > > figures out how to reproduce this. > > > > > > The question is: Is the state with this change better then the current state > > > in master? > > > > > > I would say "yes" because the mini map currently is wrong for (almost) all > > > cases. > > > What do you think? > > > > Yes, even if it more obviously broken if the restart-problem does appear. > > So what do we do? Merge this fix or not? I think we should merge
Gerrit change https://git.eclipse.org/r/126950 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=cfa89a3b2f67ee051d4ffea3ae6c8a72ed60af49
Thank you Arne for the fix.
(In reply to Till Brychcy from comment #24) > (In reply to Matthias Becker from comment #23) > > (In reply to Till Brychcy from comment #22) > > > (In reply to Arne Deutsch from comment #21) > > > > Yes, but can not reproduce it at my machine. Appart from this I think the > > > > original bug has been already fixed. Together with some others that we have > > > > found during the process. I think it makes sense to create a new bug ticket > > > > for that specific issue. Would you do that? > > > This issue doesn’t appear without your patch so i‘d say it belongs to this > > > bug. > > > Can someone with a Mac please try to reproduce this? > > > > I tried. I could not. But maybe I did not get your description correctly. I > > started a target IDE from my DevIDE and restarted this. Box height looked > > correct. > > Can you explain your steps in more details or do a screen recording? > > I tried a bit and with some editor configurations it doesn't happen. > > I general I have multiple .java files open (some of them short) plus a > .classpath plus a pom.xml > > I'll try to find to make it reproducible over the weekend. Should we keep this bug open for this issue or do you want to create a separate bug?
New Gerrit change created: https://git.eclipse.org/r/127719
> Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse? I can reproduce the problem. Please review my simple gerrit patch at https://git.eclipse.org/r/#/c/127719/ Please note the outline does that too.
(In reply to Angelo ZERR from comment #34) > > Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse? > > I can reproduce the problem. Please review my simple gerrit patch at > https://git.eclipse.org/r/#/c/127719/ > > Please note the outline does that too. the fix looks good to me. Can we / are we allowed to merge it into RC1?
(In reply to Matthias Becker from comment #35) > (In reply to Angelo ZERR from comment #34) > > > Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse? > > > > I can reproduce the problem. Please review my simple gerrit patch at > > https://git.eclipse.org/r/#/c/127719/ > > > > Please note the outline does that too. > > the fix looks good to me. Can we / are we allowed to merge it into RC1? Yes.
(In reply to Dani Megert from comment #36) > (In reply to Matthias Becker from comment #35) > > (In reply to Angelo ZERR from comment #34) > > > > Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse? > > > > > > I can reproduce the problem. Please review my simple gerrit patch at > > > https://git.eclipse.org/r/#/c/127719/ > > > > > > Please note the outline does that too. > > > > the fix looks good to me. Can we / are we allowed to merge it into RC1? > > Yes. So then I merge it today.
Gerrit change https://git.eclipse.org/r/127719 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=449c8be18610b214135255ba542a23fdb8a99bae
(In reply to Angelo ZERR from comment #34) > > Have you seen the issue in the gerrit regarding switching open editors in a restarted eclipse? > > I can reproduce the problem. Please review my simple gerrit patch at > https://git.eclipse.org/r/#/c/127719/ > > Please note the outline does that too. Just merged it. Thank you Angelo for the fix.
> Just merged it. Thank you Angelo for the fix. You are welcome!
