search

eclipse-platform / eclipse.platform

https://eclipse.dev/eclipse/
Eclipse Public License 2.0
84 stars 113 forks source link

Compare View for big files with Line Spacing freezes eclipse #1492

Closed DenisUngemach closed 3 months ago

DenisUngemach commented 3 months ago

For big files with many differences, the compare view freezed eclipse, if the line spacing was set. The reason were the necessary resources to calculate the spacing in the method getHeightBetweenLines. We have to save the heights in a list and use them to increase the performance.

Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/2143

github-actions[bot] commented 3 months ago

Test Results

 1 726 files   -  8   1 726 suites   - 8   1h 28m 25s :stopwatch: -53s  3 973 tests ± 0   3 951 :white_check_mark: ± 0   22 :zzz: ±0  0 :x: ±0  12 438 runs   - 78  12 278 :white_check_mark:  - 77  160 :zzz:  - 1  0 :x: ±0 

Results for commit 357ae154. ± Comparison against base commit cde120b9.

:recycle: This comment has been updated with latest results.

DenisUngemach commented 3 months ago

I think the newly introduced cache lineHeightsByViewer needs to be cleared in method handlePropertyChangeEvent for the scenario when the line spacing settings are changed in the preferences.

This recommendation works: if i change the spacing in the Text Editor preferences, the line drawings will be updated properly. But the call of handlePropertyChangeEvent doesn't change the spacing in the StyledTexts, , this happens in a completely different layer.

Which means this works, because the call handlePropertyChangeEvent in TextMergeViewer happens before StyledText.setLineSpacing(int) .

The actual change of the line spacing comes from AbstractTextEditor.PropertyChangeListener.propertyChange(PropertyChangeEvent)

In theory this could lead to line drawing bugs, but i wouldn't expect it.

Is this ok for everybody? It is also possible set the spacing and other line height properties directly in the TextMergeViewer.handlePropertyChangeEvent , but this is bad design, because then the StyledText-Properties will be modified twice.

It seems there is no StyledText-Listener where i can check whether the properties were changed and clear lineHeightsByViewer afterwards.

iloveeclipse commented 3 months ago

Is this ok for everybody?

OK for me. May be a comment could be added to the current listener?

DenisUngemach commented 3 months ago

Ok, done.