Remove CreateTextFromNode methods Background: SmNode have a virtual method called CreateTextFromNode, this method is overwritten in some subclasses. However, the method is somewhat buggy/incomplete, similar functionality have been implemented in the SmNodeToTextVisitor, which should be more complete. So we should remove the CreateTextFromNode methods, and reuse any useful parts, if any, in SmNodeToTextVisitor. Calls to CreateTextFromNode should be replaced with calls to SmNodeToTextVisitor, and this should be tested. Skills: Building, C++
Deteted "Easyhack" from summary
CreateTextFromNode is used only in mathmlimport AFAICS. But mathmlimport produces a structurally-wrong tree, which *can* be converted to text via CreateTextFromNode, but SmNodeToTextVisitor works only with "right" trees and fails to produce something sensible from that tree from mathmlimport. It would be better if mathmlimport could convert mathml->starmath I guess, now it does mathml->fake tree->[CreateTextFromNode]->starmath.
I have interest on it. On a first look, I saw that SmNodeToTextVisitor is a class constructor, and need an pNode and a OUString rText as argument to be constructed. But SmNode::CreateTextFromNode need only an OUString, and the pNode is created inside the method. Is that a problem? And, in SmNode::CreateTextFromNode there is a verification for the number of subNodes, and in SmNodeToTextVisitor we have this virification only in Visitor method in according with the node. Then, to replace CreateTextFromNode should I call SmNodeToTextVisitor constructor and then call the Visitor method according to the case? Thanks.
Ivan: is there still the pb of structurally-wrong tree for mathml import? Frédéric: thinking you might be interested in this, I put you in cc.
(In reply to comment #4) > Ivan: is there still the pb of structurally-wrong tree for mathml import? Most probably yes, but I forgot everything on this topic...
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility. see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
I think, that it should be a long time goal to switch from StarMath to MathML. But as long as there exists no direct editing of MathML, it is too early to remove generating StarMath source.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyInteresting SkillCpp) [NinjaEdit]
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
Missing code pointer, mandatory for easy hacks
oox/source/mathml/import.cxx (mathhtml import) starmath/source/node.cxx starmath/inc/node.hxx
If there's an assignee, let's put this tracker as ASSIGNED
A polite ping, still working on this bug
(In reply to jani from comment #13) > A polite ping, still working on this bug Hmm, Will try to submit a patch soon.
Hi, I would like to take this up as my first issue.
In which file the function "SmNodeToTextVisitor" is decleard ?
(In reply to Abhishek from comment #16) > In which file the function "SmNodeToTextVisitor" is decleard ? You can use Opengrok: See https://opengrok.libreoffice.org/search?project=core&full=&defs=SmNodeToTextVisitor&refs=&path=&hist=&type=&si=full
How can I remove these kind of warinings? "home/abhishek/core/starmath/source/node.cxx:765:47: warning: unused parameter ‘pNode’ [-Wunused-parameter] void SmRootNode::SmNodeToTextVisitor( SmNode* pNode, OUString &eText )"
(In reply to Abhishek from comment #18) > How can I remove these kind of warinings? > "home/abhishek/core/starmath/source/node.cxx:765:47: warning: unused > parameter ‘pNode’ [-Wunused-parameter] > void SmRootNode::SmNodeToTextVisitor( SmNode* pNode, OUString &eText )" It could be interesting you submit your patch on gerrit with WIP (Work In Progress) mark so devs may give their opinion. Meanwhile, without seeing more code, you can change the line into: void SmRootNode::SmNodeToTextVisitor( SmNode* /* pNode */, OUString &eText )
Actually I have met a dead end while debugging. Is it possible to submit a patch even if it isn't buliding ?
(In reply to Abhishek from comment #21) > Actually I have met a dead end while debugging. > Is it possible to submit a patch even if it isn't buliding ? Yes. You can submit it directly as work in progress using the logerrit script by: ./logerrit submit-wip master
Sorry for the delay. I have submitted the patch with commit message "WIP test 38885". Please review it.
(In reply to Abhishek from comment #23) > Sorry for the delay. I have submitted the patch with commit message "WIP > test 38885". Please review it. You forgot to link to it, but here it is: https://gerrit.libreoffice.org/c/core/+/89756
In my submitted patch there are some error messages such as "undefined references for function SmNodeToTextVisitor " but they are not showing where exactly the error is occuring? Please Help!
(In reply to Abhishek from comment #25) > In my submitted patch there are some error messages such as "undefined > references for function SmNodeToTextVisitor " but they are not showing where > exactly the error is occuring? Please Help! On your local sources, type: git pull -r then try again to build starmath module.
(In reply to Julien Nabet from comment #27) > (In reply to Abhishek from comment #25) > > In my submitted patch there are some error messages such as "undefined > > references for function SmNodeToTextVisitor " but they are not showing where > > exactly the error is occuring? Please Help! > > On your local sources, type: > git pull -r > > then try again to build starmath module. I have removed all the warning messages but there are still undefined refernces. Sorry for irrelevant commit. My current error message is /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV6SmNode[_ZTV6SmNode]+0x50): undefined reference to `SmNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV15SmStructureNode[_ZTV15SmStructureNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV13SmVisibleNode[_ZTV13SmVisibleNode]+0x50): undefined reference to `SmNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV13SmGraphicNode[_ZTV13SmGraphicNode]+0x50): undefined reference to `SmNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV11SmTableNode[_ZTV11SmTableNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV10SmLineNode[_ZTV10SmLineNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV11SmUnHorNode[_ZTV11SmUnHorNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV12SmBinHorNode[_ZTV12SmBinHorNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV17SmBinDiagonalNode[_ZTV17SmBinDiagonalNode]+0x50): undefined reference to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV15SmBracebodyNode[_ZTV15SmBracebodyNode]+0x50): more undefined references to `SmStructureNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' follow /home/abhishek/core/workdir/CxxObject/starmath/source/node.o:(.data.rel.ro._ZTV14SmPolyLineNode[_ZTV14SmPolyLineNode]+0x50): undefined reference to `SmNode::SmNodeToTextVisitor(SmNode*, rtl::OUString&)' collect2: error: ld returned 1 exit status /home/abhishek/core/starmath/Library_sm.mk:10: recipe for target '/home/abhishek/core/instdir/program/libsmlo.so' failed make: *** [/home/abhishek/core/instdir/program/libsmlo.so] Error 1
try: make starmath.clean && make starmath if still stuck, please upload your new version on gerrit by using --amend option to update the existing patch instead of creating a new one when using logerrit submit
I tried make starmath.clean && make starmath but it is still showing me the same error messages.
(In reply to Abhishek from comment #30) > I tried make starmath.clean && make starmath > but it is still showing me the same error messages. Whereas in my previous comment I had talked about amending the existing commit to avoid dups, I see there are 2 patches now: - https://gerrit.libreoffice.org/c/core/+/89938 - https://gerrit.libreoffice.org/c/core/+/89940 Both with same description: WIP 38885 removed warinings but there are still undefined referneces but not containing the same thing. 1) Abandon one of these to have only 1 patch. 2) Change description to: tdf#38885: Remove CreateTextFromNode methods Indeed description shouldn't be used to indicate the state of the patch.
(In reply to Julien Nabet from comment #31) > (In reply to Abhishek from comment #30) > > I tried make starmath.clean && make starmath > > but it is still showing me the same error messages. > > Whereas in my previous comment I had talked about amending the existing > commit to avoid dups, I see there are 2 patches now: > - https://gerrit.libreoffice.org/c/core/+/89938 > - https://gerrit.libreoffice.org/c/core/+/89940 > > Both with same description: > WIP 38885 removed warinings but there are still undefined referneces > but not containing the same thing. > > 1) Abandon one of these to have only 1 patch. > 2) Change description to: > tdf#38885: Remove CreateTextFromNode methods > > Indeed description shouldn't be used to indicate the state of the patch. Okay. Done.
Can someone help me with the merge confilct I am facing in https://gerrit.libreoffice.org/c/core/+/89938
(In reply to Abhishek from comment #33) > Can someone help me with the merge confilct I am facing in > https://gerrit.libreoffice.org/c/core/+/89938 I guess it is because you have created your work on top of this which has never been merged: https://gerrit.libreoffice.org/c/core/+/89756/ See the link "Parent 08bded6".
I am facing some errors in https://gerrit.libreoffice.org/c/core/+/89756/ Please help.
Abhishek: in https://gerrit.libreoffice.org/c/core/+/89938 Julien gave you detailed advice. Can you try it?
Yes , I tried it. I have uploaded new patch after that.There are few more errors that I am not able to understand. Please help.
(In reply to Abhishek from comment #37) > Yes , I tried it. I have uploaded new patch after that.There are few more > errors that I am not able to understand. Please help. I don't see any upload after my comment in https://gerrit.libreoffice.org/c/core/+/89938.
I was hoping to have help in https://gerrit.libreoffice.org/c/core/+/89756/ because https://gerrit.libreoffice.org/c/core/+/89938 is an indirect ancestor of it.
Also, should I abandon https://gerrit.libreoffice.org/c/core/+/89938 ?
(In reply to Abhishek from comment #40) > Also, should I abandon https://gerrit.libreoffice.org/c/core/+/89938 ? Unless, you know how to rebase, resolve conflicts, etc. the simplest thing to do would be to abandon both then: - update your local repository to be synchronized with master sources - make your changes (you can help from your abandoned patch but don't copy paste from them) - check it builds locally if ok, submit your patch to gerrit. If it's not ok and you need several days to fix this, before submitting again on gerrit, please rebase your local repo and check there's no conflicts and it still builds.
I think I have solved the conflict problem in "https://gerrit.libreoffice.org/c/core/+/89756/ ".In the current patch, I have build error named "undefined references with no particular location".
Removing easyHack keyword as this is lacking a mentor
(In reply to Abhishek from comment #16) > In which file the function "SmNodeToTextVisitor" is decleard ? Hello, are you still working on that? I was fixing some things on starmath and next was mathml. If you are already working on that would need to coordinate or let it to you if you can handle it yourself.
(In reply to dante19031999 from comment #44) > (In reply to Abhishek from comment #16) > > In which file the function "SmNodeToTextVisitor" is decleard ? > > Hello, are you still working on that? I was fixing some things on starmath > and next was mathml. If you are already working on that would need to > coordinate or let it to you if you can handle it yourself. There is also this abandoned patch (nobody reviewed it): https://gerrit.libreoffice.org/c/core/+/90171
I may import some of the code, thanks you.
dante committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/8c716704df4aaa83fcd198cc8d92cd3e1e542de9 tdf#38885 Remove createTextFromNode It will be available in 7.1.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
https://gerrit.libreoffice.org/c/core/+/98795