TCastleMemo. Upgrade TCastleEdit, TCastleLabel.

[IldusEps]

Create TCastleMemo:

1. multi-line - property Lines; Index, IndexColumn-Caret;
2. selection - property SelectedText; Function DeleteSelectText; Index, IndexColumn - first caret for selected rectangle; LastIndex, LastIndexColumn - second caret for selected rectangle;
3. Word Wrap
4. UndoSystem (in progress);
5. Scrolling (not working yet);

TCastleEdit:

1. Added to move in text;
2. selection - property SelectedText; Function DeleteSelectText; Index - first caret for selected rectangle; LastIndex - second caret for selected rectangle;
3. Ctrl+V, Ctrl+C is any position;

TCastleLabel:

Word Wrap - property Word Wrap; True - Words are carry over when they reach the edge of the field, False - not carry;

[michaliskambi]

I will take a look at it in details soon. Quick notes:

• We already had a way to wrap text in TCastleLabel (MaxWidth)

• Your PR adds a copy of CastleUnicode in src/ui/base/castleunicode.pas, please remove it

• Small note: make spaces around assignment operator X := Y not X:=Y

[IldusEps]

There are a lot of things left that were for debug. I need them for programming. But I'll delete them all later.

[michaliskambi]

There are a lot of things left that were for debug. I need them for programming. But I'll delete them all later.

Sure, that makes sense while the PR is marked as "work in progress". Thanks!

And sorry for a very quick and "random" review from me 2 days ago -- I wanted to give you feedback fast about some things I noticed, and I had limited time available. Once you mark PR as ready, I usually make a longer review where I look over everything :)

I forgot to mention explicitly that I like the features you do! So thank you for working on them, and I'm happy to see it happening.

• We indeed want TCastleMemo, and more editing capabilities with TCastleEdit.

• Where it makes sense, our UI properties/names are generally close to what Lazarus LCL / Delphi VCL calls them. I see you already follow it, and that's good.

• TCastleLabel.WordWrap makes sense, if underneath it reuses the existing wrapping code (just like MaxWidth). So WordWrap with MaxWidth would mean that we wrap to the width calculated in "default" (as right now) way, e.g. this way we could have label wrapping with MaxWidth = 0 but FullSize = true.

I would suggest heavily to limit the features you want to include in this PR. Smaller PRs are easier to create, and easier to review, easier to discuss, and in effect usually get done and applied quicker :) For example, "Undo System" for TCastleMemo can most definitely be done as a separate PR (after this one). Unless you're already deep in the middle of implementing it now.

[IldusEps]

And sorry for a very quick and "random" review from me 2 days ago

It's okay, it's even good, I will immediately know my mistakes and correct them. Moreover, a memory leak in such an important function is very bad.

limit the features you want to include in this PR

"Undo System" is already in development, some of the code is even on GitHub. Well, and I believe that TCastleEdit and TCastleLabel do not have such big updates to somehow separate them.

I forgot to mention explicitly that I like the features you do! So thank you for working on them, and I'm happy to see it happening.

I am glad to help the Game Engine, in which I use, and other people are use, of which there will be more and more.

[IldusEps]

Question: is TCastleEdit 'Undo System' needed?

[michaliskambi]

Question: is TCastleEdit 'Undo System' needed?

I would say yes. While it seems low priority for TCastleEdit, but in general sooner-or-later it will be needed by someone :), other UIs also provide undo (Ctrl+Z) in edit fields. And it is probably best to make a system that is universal (nicely fits in both TCastleEdit and TCastleMemo) from the start.

[IldusEps]

Okey!

[IldusEps]

New Function (very important, therefore, I am writing here): I know that "Index, Lastindex, IndexColumn, LastIndexColumn" - determine the position using how the text looks on the screen (that is, if the text on the screen is transferred to a new line (WordWrap), then this is already a new Index(Index+1)), but it will not be convenient for the programmer to choose the position of the caret. Therefore, now there is SetCaret, which determines the position of the Lines, not how it looks, but how the text is without WordWrap. And accordingly, will be GetCaret;

[IldusEps]

Hello! First, sorry for my missing out and working so little. There were many lessons, because I am a student.
Secondly, I have a problem, it was from the very beginning, but it did not interfere too much, but now it interferes greatly.

In general, in my TCastleMemo constructor, a TCastleScrollViewManual is created, and it is always visible in CastleEditor, which should not be, It is not comfortable, not It is convenient for moving and resizing TCastleMemo, and every time more and more TCastleScrollViewManuals are typed, they can be two or three ... but only one TCastleScrollViewManual works and is used.
But the worst thing: when deleting TCastleScrollViewManual, an error occurs, when resizing or something else- Then with TCastleMemo.

[IldusEps]

The question is: how to make TCastleScrollViewManual not visible in CastleEditor?

[michaliskambi]

As for TCastleScrollViewManual:

1. The direct answer is that you can use CastleDesignMode boolean (global, in CastleUtils unit) to avoid creating something, or to make something non-existing (like MyScrollView.Exists := not CastleDesignMode;).

   But that is really the last resort, and you should not do this. It is much better usually to show at design-time the same thing you show at runtime. You should rather solve the problems you mention :)

2. """But the worst thing: when deleting TCastleScrollViewManual, an error occurs, when resizing or something else- Then with TCastleMemo.""" -- I do not know why, but you should solve it. It is likely also a problem at runtime too, if you would try to create/delete TCastleMemo instances by code.

3. """In general, in my TCastleMemo constructor, a TCastleScrollViewManual is created, and it is always visible in CastleEditor, which should not be, It is not comfortable, not It is convenient for moving and resizing TCastleMemo, and every time more and more TCastleScrollViewManuals are typed, they can be two or three ... but only one TCastleScrollViewManual works and is used.""" -- This is also something you need to solve, it will be a problem regardless of the editor or not.

   The TCastleScrollViewManual should be created with owner set to the TCastleMemo. See TCastleCheckbox (src/ui/opengl/castlecontrols_checkbox.inc) for example how one UI control should create and "own" children, TCastleCheckbox is actually a composition of TCastleImageControl + TCastleLabel. Creating a child goes like this:

   FImagePressedBackground := TCastleImageControl.Create(Self); // make owner = Self, so it will be freed with owner
   FImagePressedBackground.SetTransient; // do not show it in editor, do not serialize it
   

[IldusEps]

"""The TCastleScrollViewManual should be created with owner set to the TCastleMemo. See TCastleCheckbox (src/ui/opengl/castlecontrols_checkbox.inc) for example how one UI control should create and "own" children, TCastleCheckbox is actually a composition of TCastleImageControl + TCastleLabel. Creating a child goes like this:

FImagePressedBackground := TCastleImageControl.Create(Self); // make owner = Self, so it will be freed with owner FImagePressedBackground.SetTransient; // do not show it in editor, do not serialize it"""

Yes, this is what was needed. Thanks!

[IldusEps]

Sorry for taking so long. My laptop was broken. I couldn't go on TCastleMemo.

[michaliskambi]

No problem :), I'm happy you still work on it. Be sure to "rebase" your PR based on current CGE "master" code -- we did a lot of updates to CGE in the meantime, make sure your code works with them.

You can follow GitHub instructions around the button "Resolve Conflicts" to do it (through web browser) or you can do it on your local system by updating your fork to follow latest CGE master. Let me know if you have some conflicts you don't know how to deal with, I can help! :)

[eugeneloza]

You can follow GitHub instructions around the button "Resolve Conflicts" to do it

In my experience those instructions are really misguiding. Unless GitHub can resolve the conflict through Web UI - then it's easy and straightforward (and yet still can end up in an unwelcome/unexpected result) - it's a much better and easier way to follow some other tutorial on resolving merge conflicts. It may get a bit more tricky in merging across the forks, but if you do it "correctly" then you understand what you are doing and not get an absolutely random result, as I usually got when I was trying to follow GitHub instructions (most often ending up in deleting the fork altogether, because it ended up in a completely unfixable state). Also merging on a local drive protects your remote branch - still it's a very good idea to make a backup before merging if you are not feeling absolutely confident about what you are doing - it's very easy to destroy the branch/fork with literally no way to undo the change.

Command-line it'll simply go as:

(when you are on your branch)
git merge "master branch name" ( - don't forget to update it to current state)
(fix the merge conflict by editing local files)
git commit -a

That's all. I have no idea how GitHub can make instructions for something as straightforward - as misguiding that usually it ends up in a disaster.

[michaliskambi]

@eugeneloza In this case it is a fork, and in the fork @IldusEps was working on master branch (not on feature-specific branch like memo).

Your instructions are for merging one branch into another within the same repo.

The way to update your fork follows
https://help.github.com/articles/configuring-a-remote-for-a-fork/
https://help.github.com/articles/syncing-a-fork/

Something like this should work:

cd .../ # enter directory where your fork, https://github.com/IldusEps/castle-engine , lies
git remote -v
  origin  https://github.com/IldusEps/castle-engine.git (fetch)
  origin  https://github.com/IldusEps/castle-engine.git (push)
git remote add upstream https://github.com/castle-engine/castle-engine.git
git remote -v
# you should see 4 lines now, with "upstream"

git fetch upstream
git checkout master
git merge upstream/master
# this will fail with conflicts; fix them locally, and commit "git commit..."
git push

I fully agree that it's best to just make a backup (copy your working state) before doing it, if you don't feel certain about it :)

That said, I would try first the GitHub "web editor" to resolve conflicts -- it does the job well in simple cases. And you can just "cancel" it if it is not comfortable/useful.

[IldusEps]

Fimages and FCorners were replaced byFImagesPersistent?

[IldusEps]

Okey. I'll check everything

[eugeneloza]

Fimages and FCorners were replaced byFImagesPersistent?

Yes. You may check #254 for details.

[michaliskambi]

This PR is a bit old, and I'm not sure is it still active.

In the meantime, we have done some things in CGE master related to editing -- in particular support for on-screen keyboard on Android.

We also have a roadmap item summarizing the needed work around TCastleEdit and TCastleMemo (they are connected), https://castle-engine.io/roadmap#edit .

So, question: is the work on this PR still active? If yes, my first advise would be to synchronize with latest CGE master. I know it will be some work, but sooner or later it will have to be done :)