Unicode display bug with expressions

[realthunder]

@realthunder, could you please have a look at my initial post? Maybe you can help to fix this issue or tell me where in the code I have to look?

Expression access the property through API PropertyContainer::getPropertyByName(), so it shouldn't matter if it is py2 or py3. If we want to do it now, just lift the ASCII only naming restriction in DynamicProperty::addDynamicProperty() and PropertySheet::isValidAlias() should be fine.

On the other hand, lifting property name restriction may be to invasive IMO, and may cause unnecessary compatibility problems. With my branch (also included in my expression completer PR), I have changed how Spreadsheet provides cell alias. Instead of adding a dynamic property, it literally gives the same property an alternative name, and expose it through getPropertyByName(), and can be enumerated using new API PropertyContainer::getPropertyNamedList(). With that, we only need to lift naming restriction in PropertySheet::isValidAlias(). I just tried it in my branch, and it works fine. Here is the commit, which includes a few other minor changes that I can think of.

Screenshot from 2021-01-26 09-41-16.png (17.67 KiB) Viewed 1079 times

[wmayer]

So when I got it right, I can use Python's isidentifier() command to check if an alias is a Python identifier.

This alone is not sufficient. You also have to make sure the string is not a keyword. E.g.

Code: Select all

"for".isidentifier() # returns True

So, you have to do

Code: Select all

import keyword

def isidentifier(s):
    return s.isidentifier() and not keyword.iskeyword(s)

Werner, should I stop or would yo take over or should I try to go on in perspective for FC 0.20?

Not sure what exactly you mean. But IMO in regard for v0.19 the support of allowed identifiers shouldn't be changed. This something to be discussed for v0.20 since it might cause some unexpected problems here and there.

[uwestoehr]

Here is the commit, which includes a few other minor changes that I can think of.

Many thanks. Your PR is big and changes so much that I could not just take over your idea to the existing expression mechanism. Could you make a smaller PR that just adds the Unicode capability with the current expression method?

Maybe this is also a way so split your big PR into smaller pieces that people can review better - first smaller PRs that change everything that can be done with the existing expression engine, then finally the big change that only holds the changes really necessary for the method change. I think this way not only the review is easier but also those who reviewed the smaller PRs become automatically familiar with the Expression code and can then also review the big change.
I hope you are not annoyed by this proposal, I only see that your work and features are amazing and I would like to have them in FC as soon as possible but as the review process is stalled (most devs including myself cannot handle such big PRs), we must find a way to make the review happen, therefore my proposal

Concerning your commit, why did you change the QString to a std::string in SpreadsheetView.cpp? I mean the QString holds the string from the UI field already as Unicode so it is not clear to me why we should convert it to a std::string for further processing.

[uwestoehr]

So, you have to do...

Thanks.

Werner, should I stop or would yo take over or should I try to go on in perspective for FC 0.20?

Not sure what exactly you mean.

My question is if I could go on trying to implement Unicode support for aliases for FC 0.19, or not for 0.19 but 0.20 or I should wait until FC 0.19 is released?

[wmayer]

or not for 0.19 but 0.20

For v0.20. We should soon come to an end with v0.19 and thus no highly experimental code should be added any more.

I should wait until FC 0.19 is released?

Yes.

[uwestoehr]

or not for 0.19 but 0.20

For v0.20. We should soon come to an end with v0.19 and thus no highly experimental code should be added any more.

So I will be patient.

[realthunder]

Here is the commit, which includes a few other minor changes that I can think of.

Many thanks. Your PR is big and changes so much that I could not just take over your idea to the existing expression mechanism. Could you make a smaller PR that just adds the Unicode capability with the current expression method?

The relevant commit for changes in Spreadsheet cell alias is here. I usually try to split my PR in fine commits (see the completer PR here), and with explanatory commit message if the commit is big.

Concerning your commit, why did you change the QString to a std::string in SpreadsheetView.cpp? I mean the QString holds the string from the UI field already as Unicode so it is not clear to me why we should convert it to a std::string for further processing.

Because FC primarily works with std::string, and QString::from/toUtf8() are not exactly cheap. The most important change in that part is to use Base::Tools::escapedUnicodeFromUtf8(), mostly for Python2 I think.

[uwestoehr]

The relevant commit for changes in Spreadsheet cell alias is here.

Ah, thanks. However, I am a bit confused since the commit you posted yesterday was for your private repository (I guess Link stage 3) and there the changes are massive.

I usually try to split my PR in fine commits (see the completer PR here), and with explanatory commit message if the commit is big.

Hmm, the PR consists of 28 commits. Could you please squash them to one and rebase? I would try it out but I cannot merge it with master. Kindly @chennes and @ezzieyguywuf reviewed it and I guess it would also make it easier for them to continue.

Because FC primarily works with std::string, and QString::from/toUtf8() are not exactly cheap. The most important change in that part is to use Base::Tools::escapedUnicodeFromUtf8(), mostly for Python2 I think.

I am no expert but since the UI strings are QStrings, it seems we won't too save much when we have to perform at least 2 conversion from and later to it. In other fields of FC like TechDraw, I see that QString is consequently used.
Since Py2 will be dropped and Werner stated that the alias change won't make it to FC 0.19, I think we can directly get rid of it and modify the PR with target 0.20 I think.

[realthunder]

Hmm, the PR consists of 28 commits. Could you please squash them to one and rebase? I would try it out but I cannot merge it with master. Kindly @chennes and @ezzieyguywuf reviewed it and I guess it would also make it easier for them to continue.

I think squash would be harmful. As you can see from my reply to their comments, the commit message is there for a purpose. I did made a few minor adjustment according to their comments. I haven't rebased it yet, because I am not sure if it will disturb the links in those discussion.

I am no expert but since the UI strings are QStrings, it seems we won't too save much when we have to perform at least 2 conversion from and later to it. In other fields of FC like TechDraw, I see that QString is consequently used.

QString internally uses UTF16, so converting to UTF8 still require some work. That being said, performance is not the primary concern in those changes. It also saves some typings, if you ask me, lol.

[uwestoehr]

I think squash would be harmful. As you can see from my reply to their comments, the commit message is there for a purpose. I did made a few minor adjustment according to their comments. I haven't rebased it yet, because I am not sure if it will disturb the links in those discussion.

OK, however, then I still did not get our policy. When I had just 3 commits per PR people requested me to unite them and to rebase. Thus I do this now for my PRs.
However, for your PR it is important that it will go in and I thought a rebase would help because then you can just apply the diff and try it out with current master instead of checking out the branch of the PR.

That being said, performance is not the primary concern in those changes.

I am not sure about this, because I learned here in the forum what people like e.g. @kbwbe do - huge assemblies with tons of expressions and I think the new expression method should therefore be optimized also in terms of performance. I mean we cannot know yet what people will/can do with FC in future.
Also, I am not opposed to your QString -> std::string conversion, I only wanted to understand the reasoning and of course to learn something.