Closed alpyre closed 8 years ago
Thanks for this new pull request. However, as I stated in issue #9 I am still not convinced that MUIA_TextEditor_Slider_Behaviour should be added alltogether. I really can't think of a single reason why one would have to have a resizing horizontal slider and thus we shouldn't provide this functionality at all. In addition, calling this behaviour "MUI" is definitly not right since this has really nothing to do with scrolling "the MUI way".
I saw that you made this attribute private. However, If something is private, its private and shouldn't be used by anyone outside TE.mcc at all and if we don't use this functionality internally in TE.mcc itself I don't see a reason to keep that functionality at all?!?!
For example, a developer may want to create a gadget to display some data (which is formatted text), and could prefer to create a TextEditor gadget in ReadOnly mode to benefit from its great import and styling features and would like to see its horizontal scroller behaviour match the behaviour of an NListView one...
That would be very nice if such a (maybe rare) situation was already covered.
...and actually I would prefer the attribute to be public, but if not, at least someone who may need such a feature could find it in the sources and be happy. :)
NOTE: BTW, the way NListView and NListTree use their horizontal scrollers is non-standard as well. In no other platform I saw such a behaviour. That's why I call it the "MUI way". And if we will remove this little feature from TextEditor just to match standards, we should also remove it from NList classes as well.
That is how I see the situation.
If NList is really doing the same non-standard way of recalculating the size of the horizontal slider upon scrolling down vertically I think it should be changed as well, yes. As I partly know the NList sources I think this might very well being a small limitation due to the fact that the NList sources are so hard to maintain that the original authors simply fail to implement this functionality. So if you have some knowledge now how/where to implement/change the behaviour please go ahead and provide a pull request for the NList sources as well ;)
And keeping this attribute private potentially allowing developers to exploit that (unwanted) feature is not really the way an API design should work. private Tags should NEVER be used by third party developers no matter what or how "cool" these developers might think this could be. Otherwise substantial functionality might break as soon as internal APIs are changed.
So for the time being I think I have to insist that you please remove this attribute and only support standard scroll handling in TE.mcc for the horizontal slider. However, feel free to add verbose comments to the code passages in case we need to revisit this situation again and potentially implement such an attribute again for public use.
@tboeckel What is your opinion/experience on that?
From my point of view NList's behaviour is definitely wrong, or better: a compromise for the sake of speed.
The reason is simple. To calculate the width of the longest line one simply has to iterate over all lines and calculate the longest. For lists with a large number of entries this might take a considerable amount of time. I think this is why Gilles Masson oriiginally chose to respect the lines in the visible area only. Of course this is LOTS faster, but results in a wronly scaled horizontal scrollbar.
For TextEditor.mcc the situation is exactly the same. To get a correctly scaled horizontal scrollbar all text lines have to be taken into account, and the calculation might cause considerable slowdowns while editing a line. Of course this can be optimized, i.e. by caching the calculated line width for each line and recalculating the width of the currently edited line only. Nevertheless the maximum of all cached width values must be recalculated upon each edit operation which might have affected the width of the current line. Perhaps there might be room for further optimizations, but eventually the full calculation must be done and will take its time.
MUI's own list class does in fact take the tedious way and calculates the widths of all lines if it really has to. This is the reason for this comment in the source:
check every single entry. very slow for lists with many entries
Thanks for your comments @thore. So I would propose to keep the standard behaviour of scrolling active and see which experience users will make once TE.mcc with HSlider is released.
And of course it would be good to see if we can further optimise the calculation of the longest line. And probably one should do the same for NList and see if we can restore the standard way of calculating the HSlider size while optimising the calculation of the longest line.
I've quickly tested my standard behaviour implementation on my MorphOS creating more than 2000 lines and I have NOT experienced any slowdowns when editing text. However it is a 1.42 GHz G4 Mac and this may be expected. :)
Unfortunately I am unable to compile for OS3 and OS4 now... :(
I can test this on OS3 and OS4.1 Classic with a 330/60Mhz A1200 as soon as a release is available and then consider if any further optimization is vital or not.
As a final additional note here, I've now noticed that the most popular Amiga IDE CubicIDE has the same horizontal scroller behaviour. ;)
The argument that other applications are doing things wrong in the same way doesn't make the situation more ok or correct. This is like "shit tastes good, millions of flies cannot be wrong" :)
Taking just the visible lines into account is faster and easier, but it is definitely not intuitive.
I have to object to "it being not intiutive".
In the case of an IDE it may even make sense. Actually who would want to be able to scroll into an area on the right whilst there is nothing there, just because of a longer line hundreds of lines above or below the area you are working on?
Also you can get the information on the "length of the lines in the area you are currently working on" by just looking at the horizontal scrollers current state.
I mean, such a behaviour can even come in handy in some cases. I am not for or against the feature. I just see it as a preference that should be covered. ;)
Finally I could have set the working build environment for OS3 up on linux (I'm afraid I lost some more of my hair in the process - why this stuff is this complicated!?).
Ok. On my 060 A1200 you feel no slowdowns up to 10.000 lines. But at about 12.000 lines it starts to feel itself after 15.000 it starts to become frustrating.
What do you think about these numbers?
I am frauf but I think a real A1200 is not really our main targetted platform anymore as I can hardly believe that someone (or a significant number) is still using this slow machine as their main computer system anymore. Our main platform we want to support is a at least a modern PPC driven system under OS4/MOS or an UAE driven system like FS-UAE or WinUAE using OS3. In addition I believe the longest line calculations can certainly be improved.
So I would say for these few people still using real amiga they can live with this limitation (or the situation being improved by redesigning the calculations) and for the majority of amiga users which are using modern systems they won't notice any slowdowns really.
10000 lines are not bad, I think. Of course things become slower with every additional line, this is the price one has to pay. Furthermore the calculations are necessary for non-wrapped lines in combinations with a horizontal scrollbar only. It TE.mcc is used to implement a simple text editor I'd guess that more than 10000 lines per file will occur quite seldom only, although this case cannot be excluded. Perhaps we can add an approximation for very large files instead of doing exact calculations.
To give a reference turkish.po (of YAM) is a file of 10.000 lines.
From it being still editable on a real Amiga without noticable slowdowns, I think we can say that the algorithm is fast enough.
However we surely can have enhancements. Ideas are: 1) To cache the textlengths for every line (as Thore mentioned). But this would spend additional 40kB's from memory in the case of a 10.000 line... ...or... 2) To calculate the longestline by traversing all the lines ONLY in the case of an import. In the cases of other changes, calculate ONLY for the edited section, and compare it with previous longestline value.
I find the 1st one easier to implement but the 2nd one more optimised and intelligent. Though for the 2nd, I will have to dig deep into EditorStuff.c and understand every bit of it first.
So I'd prefer to postpone this for a while (with your approval), since we have more important issues for now. ;)
I fully agree. So please pospone this for a later time and concentrate on integrating #12 and #13 instead which are required for a 15.48 release. However, please make sure to open a new ticket for improving the calculation of the longest line how you proposed so that we don't forget about it.
I just recognized that other editors on Windows and Linux suffer from exactly the same problem. I.e. Geany, which is based on Scintilla, initially respects the visible lines only. As soon as a longer line becomes visible (i.e. by scrolling down) the horizontal scrollbar is adjusted, but only if the line is really longer than the longest line displayed so far. If all visible lines are shorter again the scrollbar is not adjusted.
BUGS: In PrintLineWithStyles.c, PrintLine() function:
Previous calculation of maxwidth variable was causing glitches with larger font sizes. This is corrected. (361 and 453) A long existing implicit bug in text printing loop, which revealed itself with the horizontal scrolling implementation now removed. (376) A calculation mistake in separator drawing corrected. (472) FEATURE: A new private attribute MUIA_TextEditor_Slider_Behaviour added with two values: MUIV_TextEditor_Slider_Standard MUIA_TextEditor_Slider_MUI.
In standard mode the horizontal scroller reflects the current longest line within all text (as all modern text editors do). In MUI mode it reflects the longest line within FOV (Field Of View).