Lots of (re-)implementations to support a horizontal scroller.

[alpyre]

• Lots of (re-)implementations to support a horizontal scroller in NoWrapMode.
• TextFitNew() and TextLengthNew() are re-implemented to support very large input/return values and instead of taking a dummy TextExtent, TextFitNew() now takes a TextExtentNew structure which uses LONG type members and fills it properly. So that its te.Width member can be used instead of calling another TextLengthNew(). This provides very good optimization.
• Cursor being drawn out of gadget boundaries issue when in NoWrapMode resolved.
• Issue of highlighted text changing color when edited resolved.
• Experimental MUIA_TextEditor_HorizontalScroll now acts as a switch for turning off horizontal scrolling even in NoWrapMode.
• Behaviour of FLG_HScroll re-implemented (now keeps the state of MUIA_TextEditor_HorizontalScroll attribute).
• New public attribute MUIA_TextEditor_HorizontalScroller.

[jens-maus]

Thanks again for this nice pull request. Here are some comments/requests however to slightly change your source to improve certain things:

1. Please remove all your // Alpyre type of comments. this is what a revision control is all about and with a simple "git blame" anyone can find out who last changed a line or not. So these comments are not required and obfuscate the code only. So please remove them.
2. Please explain the difference between MUIA_TextEditor_HorizontalScrolland MUIA_TextEditor_HorizontalScroller. Why are two attributes required? and why have you defined MUIA_TextEditor_HorizontalScroll to be 0x2d? In addition, the naming of these new attributes seem to be somewhat awkward. A better naming would be MUIA_TextEditor_HorizontalScrolling and MUIA_TextEditor_HorizontalScrollbar
3. Please use _mwidth() instead of saving the value in a variable throughout your changes. _mwidth() is a macro that should always be used. Please also do so for all the other _XXXX() MUI macros in your changes.
4. Please use proper source code indentation and keep the original code style preserved. That means, e.g. that you shouldn't write if (blah) but if(blah) without any space between ifand (. Also you should put variable declarations on the next line after { and not doing something like { LONG prop_entries; like in line 935.
5. line 982: please put '{' on the next line and please check all your changes for similar situations.
6. line 565+566: what does this "quick fix" actually do? What happens if you remove these lines again?

So please adapt your changes according to these comments and then I will continue reviewing your changes and give some more comments before I will integrate/merge your pull request.

[alpyre]

You're wellcome.

1) Done. (all "// Alpyre" comments removed.)

2) Firstly MUIA_TextEditor_HorizontalScroll was previously there in the code with the value 0x2d. I think one of the first coders on the project (probably Allan Odgaard) put it there with the idea to implement horizontal scrolling later. All that it actually does is to set/get the FLG_HScroll flag to/from data->flags. This then is used only for querying some conditions which could be respectively achieved by querying MUIA_TextEditor_WrapMode for NoWrap. So I replaced those flag queries with MUIA_TextEditor_WrapMode queries, put the FLG_HScroll into new use (which is to switch horizontal scrolling on/off when in NoWrapMode). Your idea to name the attributes as MUIA_TextEditor_HorizontalScrolling and MUIA_TextEditor_HorizontalScrollBar is quite reasonable. So I did it.

3) Well, I understand you. It may be a rule to do it that way, but I have to insist that this could provide great speed optimization if those macros are evaluating to functions (ie. to a get() function) and if not it wouldn’t be that much a loss. Btw. I changed them to be const variables now which makes sense.

4) All Done.

5) Done.

6) If your remove them, the text color used drawing the cursor on highlighted text does not match the highlight color. This was a long existing bug. With this fix, it is resolved. It is a “bad quick fix” because it would be better if it was fixed inside SetColor() function. I just copy pasted some part the highlight implementation used in PrintLine() function. (This may still be the only way to fix it. To figure out we should first examine and understand how SetColor() function works which I didn’t yet).

[jens-maus]

@alpyre

First of all, thanks for adapting your pull request. It is closely getting into a shape that I can start accepting it soon.

Here are some more questions/remarks for you:

1. Thanks for deleting the // Alpyre comments.
2. Honestly, I still don't understand why you still require two different attributes for horizontal scrolling. While I understand the meaning for MUIA_TextEditor_HorizontalScrolling to simply turn it on/off I still don't get what the second attribute is all about?
3. Regarding the _mwidth() type of macros I still have to instist you still please change them back to direct use where appropriate. Obviously you are missing that they are really just macros, See here: https://github.com/amiga-mui/texteditor/blob/master/include/libraries/mui.h#L3499 And then even if they weren't macros the speed improvement isn't really so high on modern amiga systems and we are not considering real OS3/m68k hardware to be the target platform for TextEditor.mcc anymore.
4. Thanks
5. Thanks
6. If so, then please open a dedicated ticket (https://github.com/amiga-mui/texteditor/issues) for this type of problem and reference the relevant problem there please so that we don't forget about it.
7. line 936 ff.: Please fix source code indentation
8. Please explain the meaning of the MUIA_TextEditor_HScroller_XXX private attributes you added.
9. line 361: please remove all spaces at the end of the line
10. line 463: remove empty line
11. line 704: remove all spaces at the end of the line
12. line 435 ff.: please fix source code indentation style.
13. line 828+894: add space before return()
14. line 340: add separate value definitions for all the LONG values and don't use "," to concatenate definition after another. This is bad coding style. Thus, each LONG value on a separate line starting with "LONG".

And please make sure to not add further changes to your master branch that are not related to the horizontal scrollbar change proposed here. Every change of functionality should be put into separate pull requests instead referring different git branches.

[alpyre]

2) There is only one new public attribute which is (with its new name) MUIA_TextEditor_HorizontalScrollBar. Which is a "Set Only" attribute to connect the gadget to a Horizontal Scrollbar object (similar to the MUIA_TextEditor_Slider). The other one (MUIA_TextEditor_HorizontalScroll - now renamed as MUIA_TextEditor_HorizontalScrolling and made public) was already in the code which was implemented only to have the same functionality with MUIA_TextEditor_WrapMode (which was something quite unnecessary). Now I put it into use for switching horizontal scrolling on/off. For example: a developer may want a texteditor that has NoWrapping and also does not scroll horizontally either. Such a gadget can be set with this attribute. (It is switched on by default.)

3) I am convinced. Done.

6) Done.

7) Done.

8) - MUIA_TextEditor_HScroller_Pos is xpos of gadget. (its notifications are connected to the MUIA_Prop_First of the scroller.)

• MUIA_TextEditor_HScroller_Vis is _mwidth(object). (its notifications are connected to the MUIA_Prop_Visible)
• MUIA_TextEditor_HScroller_Ent is the current longest line (in pixel length). (its notifications are connected to the MUIA_Prop_Entries)

9) Done 10) Done 11) Done 12) Done. 13) Done. 14) Done. Allright.

[jens-maus]

In addition to the last bunch of requested changes above please also make sure to adapt the Autodocs (https://github.com/amiga-mui/texteditor/blob/master/doc/MCC_TextEditor.doc) and please add the new attributes you have created there with a proper developer documentation.

[alpyre]

Would you like me to also change the private attributes MUIA_TextEditor_HScroller_XXX as ..._HSlider_XXX ?

[jens-maus]

Yes, you can also change HScroller to HSlider

[alpyre]

Good morning. I belive we are there at last. I've made the required additions to:

• ChangeLog
• doc/MCC_TextEditor.doc
• doc/MCC_TextEditor.readme

I hope it is all good to go.

[jens-maus]

Thanks for all your patience. I have now successfully merged your changes (after having squashed them into a single commit) to the master branch of TextEditor.mcc. Next would be to actually test it throughoutly and then prepare a new release soon.

[alpyre]

You are all wellcome. Thanks for the merge. It was tiresome but fun and very educative for me at the same time. I'll pay more attention to code styles in my future commits. ...and before I forget... ...so many thanks for all that you've done for the Amiga platform through all those years.