New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
App/Gui: Refactor expression completer and unit expressions #3062
base: main
Are you sure you want to change the base?
Conversation
996ed50
to
64fdd50
Compare
It seems the Travis tests fail because of the Spreadsheet unit tests. Maybe you can re-write these as well, or comment them temporarily, so that Travis finishes testing the rest of the code. |
That's strange. I ran the test several times to make sure it'll pass unchanged. I'll investigate. |
@vocx-fc Can you try build my branch and see if you have problem with the Spreadsheet test? @wwmayer Can you shed some light on this? Anything special about the running test in Travis? I have built Python2 and 3 on both Linux and Window in three computer setup, all of them can pass the spreadsheet test. |
Have you tried to run the command line version? When only testing with the GUI version then additional modules are loaded and things apparently seem to work while in cmd line version they fail. |
Yes, I tried command line version on all of my computers. |
I fail to compile your branch. It errors very soon.
I see your branch is a bit old, based on 16b2b9a. I tried to rebase it onto master but I get conflicts.
|
Another strange issue, maybe they are related. How did you clone my branch? My initial submit of this PR had this build error. I force pushed the fix along with an upstream rebase. Maybe travis got stalled cache or something because of the force push? But then again, how did you get the non-existent revision? Please check the header file src/Base/Unit.h. The fixed version shall include additional header . |
I |
Hmm... maybe it's github getting some cloud syncing problem. I don't know what did you see, but on my computer viewing this very PR, it shows my force push at 22 hours ago, and your first comment 17 hours ago. So, by right, you shouldn't be able to get the old revision, unless you have cloned my branch very soon after my PR submission? |
I think what happened is that I added your repository earlier, around the time you made the pull request, but I had not |
I compiled successfully, opened FreeCAD, tested the Spreadsheet, and everything worked fine. I ran the unit tests of Spreadsheet and these are some of the messages that I get. They appear on the terminal, but they don't make the tests fail.
|
4689686
to
f7f84c0
Compare
e51749f
to
648f1b4
Compare
648f1b4
to
04bd777
Compare
2886179
to
39cd5c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thanks and kudos to @realthunder for writing this -- I love the functionality, it makes expressions much faster to write and much less error prone. I also appreciate the "discoverability" that this feature adds to FreeCAD, allowing me to easily see what properties are available where. I've added a few very minor comments within the code. I cannot pretend to understand all of what I am reading here, I am still learning the FreeCAD codebase, and this is a pretty daunting change to look through! So my review is mostly that it compiles and runs, and works as intended, and I don't see anything too horrifying in the code. Well, seeing a union in modern C++ scares me a little... :) I know, that was there before...
@@ -130,7 +130,8 @@ const boost::any Property::getPathValue(const ObjectIdentifier &path) const | |||
|
|||
void Property::getPaths(std::vector<ObjectIdentifier> &paths) const | |||
{ | |||
paths.emplace_back(getContainer(), getName()); | |||
(void)paths; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment about why getPaths does nothing would be nice - I don't understand why it's been disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is mentioned in 3b5388d commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, I was about to repeat myself here! Can you please add a short comment in the source code, so readers don't have to dig through git blame?
src/Gui/ExpressionCompleter.cpp
Outdated
if(role == Qt::EditRole) | ||
return QString::fromLatin1(".%1.").arg( | ||
QString::fromLatin1(sobj->getNameInDocument())); | ||
else if(role == Qt::UserRole && obj->getPropertyByName(sobj->getNameInDocument())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a good idea to give Qt::UserRole (and Qt::UserRole+1, which I think is also used later on) more descriptive names within this file, either via a locally-defined enumeration, or just some const ints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I'll change that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes! I started this review but never hit submit (months and months ago).
I'm going to go ahead and submit this - this review is incomplete but hopefully it can begin some good conversations
src/App/Property.cpp
Outdated
void Property::getPaths(std::vector<ObjectIdentifier> &paths) const | ||
{ | ||
paths.emplace_back(getContainer(), getName()); | ||
(void)paths; | ||
// paths.emplace_back(getContainer(), getName()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this function is not being used any more - why not just remove altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is still used by some property to expose its sub-value to be accessed by the expression only. The property can choose to expose sub-value either through Python, which is also accessible by expression.
|
||
// Unlike the above path (which provides units that are not available | ||
// through python, the following paths provides the same value. They are no | ||
// longer needed, because the expression completer will now dig into all | ||
// python attributes. | ||
|
||
// paths.push_back(ObjectIdentifier(*this) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Rotation")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Axis")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("x"))); | ||
// paths.push_back(ObjectIdentifier(*this) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Rotation")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Axis")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("y"))); | ||
// paths.push_back(ObjectIdentifier(*this) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Rotation")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("Axis")) | ||
// << ObjectIdentifier::SimpleComponent(ObjectIdentifier::String("z"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than commenting this out, it should be deleted altogether.
Then, this could be a standalone commit, explaining in the commit message what you have here in your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit 3b5388d that did the change mention the reason in its commit message. The modification here just provides some more detail with context.
src/App/ObjectIdentifier.h
Outdated
@@ -69,6 +69,7 @@ AppExport std::string quote(const std::string &input, bool toPython=false); | |||
_t(_t &&other) { *this = std::move(other); }\ | |||
_t &operator=(_t &&other) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this blank line added?
src/App/ObjectIdentifier.h
Outdated
@@ -174,10 +183,13 @@ class AppExport ObjectIdentifier { | |||
int begin=INT_MAX, int end=INT_MAX, int step=1); | |||
|
|||
static Component SimpleComponent(const char * _component); | |||
|
|||
static Component SimpleComponent(const String & _component); | |||
static Component SimpleComponent(const String &_component); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a trailing space on this line - it should be deleted.
static Component SimpleComponent(const String &_component); //<- the trailing space is here after the ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep submitting my reviews as I finish them. Please let me know if you'd prefer for me to wait until I'm done with everything before submitting.
src/App/DynamicProperty.h
Outdated
@@ -75,6 +75,8 @@ class AppExport DynamicProperty | |||
//@{ | |||
/// Get all properties of the class (including parent) | |||
void getPropertyList(std::vector<Property*> &List) const; | |||
/// get all properties with their names | |||
void getPropertyNamedList(std::vector<std::pair<const char*,Property*> > &List) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the getPropertyMap
contain all this same information?
Why is it necessary to have this info in a std::vector<std::pair>
rather than the std::map
that is already provided?
For example, I see this is used in ExpressionParser.cpp but couldn't that range-based for-loop loop over the key-value pairs in the std::map
that is already provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit bdc1c835 introduced this API and mentioned the usage in its commit message. It is for property aliases. Originally PropertySheet just add a real duplicated property with a different name as alias, which is a waste of resources.
@@ -71,7 +71,10 @@ struct AppExport Expression::Component { | |||
class AppExport UnitExpression : public Expression { | |||
TYPESYSTEM_HEADER_WITH_OVERRIDE(); | |||
public: | |||
UnitExpression(const App::DocumentObject *_owner = 0, const Base::Quantity & _quantity = Base::Quantity(), const std::string & _unitStr = std::string()); | |||
|
|||
static UnitExpression *create(const App::DocumentObject *_owner, const char *unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This static method seems unnecessary. I can see that it is being used in the parser (yacc), but it is simply a workaround due to a misuse of Flex/Yacc.
There is a very nice overview here, but to summarize, the lexer should parse an arbitrary string into tokens.
In this case, the lexer should determine whether or not a given string is a valid UNIT.
Next, the lexer can assume that any UNIT it recieves is valid, since the lexer took care of the grammar. The job of the lexer is to ensure that the tokens created by the lexer are put together in a valid order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit d1e6fe6 introduced this change and explained the reason behind it. When new units are introduced (which happens quite often), the name may clash with existing property names or aliases. It is possible to disambiguate the meaning of the token under the usage context. However, the lexer has no concept of context, which is why it is not suitable for interpreting unit here.
@@ -101,7 +106,7 @@ class AppExport UnitExpression : public Expression { | |||
|
|||
private: | |||
Base::Quantity quantity; | |||
std::string unitStr; /**< The unit string from the original parsed string */ | |||
const char *unitStr; /**< The unit string from the original parsed string */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this type been changed?
I can see that the UnitExpression constructor has been updated such that it takes 2 rather than 3 arguments, with unitstr
defaulting to 0 in a protected constructor when needed but...why?
What is the purpose of this change? It seems to obfuscate and make things more difficult than they need to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of commit d1e6fe6 is to concentrate all unit string in one place as constant string, for both parsing and printing. Any unknown unit will be interpreted as a VariableExpression. So there is little reason to use std::string here.
@ezzieyguywuf Please continue. I'll reply your comments later. |
@realthunder saw your replies to my comments, thank you for taking the time to read through them. I will find some time to review your replies and provide some responses of my own, as well as to continue the code review (i had stopped earlier to give you a chance to catch up) |
a1c4e0c
to
de1ef61
Compare
fixed ... FreeCAD/FreeCAD-CI@a11fb14 and improved .... FreeCAD/FreeCAD-CI@fb0f2a9 |
this has been pending for quite a while. what's needed to move this forward (other than fixing some merge conflicts that have accumulated during the delay, of course)? |
PR suspended for now, and will resume after topo naming merge. |
i know this is on hold but for the record I get an error when compiling:
Adding |
@realthunder whenever you come back to this please take into account the feature introduced with #7688 and adapt it to use the new syntax to reference constraints that have a name that isn't a valid identifier, if I'm not mistaken it is something like |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Could this also be resumed now, after the recent toponaming merges? |
Forum thread https://forum.freecadweb.org/viewtopic.php?f=17&t=43412
The refactor are mostly in Gui/ExpressionCompleter.cpp file. The misleadingly huge change set is due to some minor change in expression scanner and parser source file, which resulted huge changes in generated code.