Dear Edward, I find that it was very difficult to discuss code improvement ideas with you. The most noticeable road-block for such discussions was your habit of not reading and therefore not understanding the idea. Of course it may be just my inability to express the idea clearly enough. After all I am not a native English speaker. In most cases our discussions were ended with you claiming that everything is sound and good enough in Leo's code. In your recent ENB post about Leo's outline redraw code you wrote again some claims that I truly believe are wrong. For as long as you keep saying such claims to yourself there won't be any serious improvement possible. If you really want for us to have a meaningful discussion you have to be open to hear some critiques and to take necessary time to fully understand what the critique is about.
Recently someone on this list wrote some critique using poorly choice of words like "bad as usual" and similar. Your reaction was to almost ban him and there wasn't any further discussion. I can understand that such critiques are painful but you have to separate yourself from Leo's code and not take those critiques as if they were saying that you are bad. I am certain that nobody on this forum would feel for you any less than pure admiration for everything you do, for the way you write and share your thoughts with us and for all the hard work that you have done so far creating Leo. But in spite of our collective admiration for you as a person, there are some bad smells in Leo's code. OTOH when you start to defend Leo's code you don't hesitate to use words like "immature" or "lack of experience" regarding your opponents which is not very helpful either. You were recently looking for a juicy problem to solve. I think there are quite a few juicy refactorings that should be done to improve Leo's code. However it seems that you are not particularly fond of refactoring Leo's code. Our communication was sometimes enjoyable and successful like when we recently discussed deletePositionsInList method. To me the code looked obviously correct. I couldn't write any clearer explanation. The curse of knowledge left me helpless. Then you took your time and you dived into the problem and proved yourself that the code is correct. I would like if you do this for any code improvement idea we discuss. Take some time and dive into the problem before rejecting the idea or critique. Now let's get back to your post about outline redraw code. You wrote: It's all too easy to lose data when switching from one node to another. > and I would agree with this claim. It is too easy to lose data. However I believe you meant to say that this is the fact that can't be changed. To me this is true at the moment in Leo's present state, but it is not something we can't do anything about. Rather to me this is the code smell. This is something that needs to be fixed. In the past I have written several implementations of Leo's outline in different languages and GUI frameworks. In all cases I made very simple and clean way to select nodes without any data loss. You could say that those were just experiments and that this doesn't necessarily apply to Leo. But I am certain that Leo's tree select can be greatly simplified and improved providing that we make all necessary refactorings. I almost hear you saying that the selection code is complex because it has to be and there is nothing we can/should do about it. But I truly believe this argument isn't true. You just need to dive into the problem and take some time to think it over and you'll see that it really doesn't have to be that complex. One of the biggest sources of complexity in this area is Leo's GUI wrapper/widget machinery. Over the years you have convinced yourself that this is the good idea. It separates Leo's core from the GUI and allows different GUI's to be attached to Leo's core. But that just isn't so. The benefits of this idea are not so big and yet the complexity it introduces is considerable. The separation is not perfect either. There are some leaks from GUI into the core. And it introduces one whole extra layer that makes reading and understanding code very hard. If Leo was written in Java or similar language this architecture would be necessary but in Python it doesn't provide anything substantial. The wrapper classes are like Java interfaces or Scala/Rust traits. However in Python those are not necessary at all. The main benefit of this classes is for writing a new GUI, but how often does it happen? Is it worth the effort? I really doubt it. Suppose we want to write a new GUI for Leo. The way you would do it I believe is to copy all wrapper classes in a new module and then start to implement methods that must be implemented. But you could also take qt classes and copy them into the new module and then replace the implementation of methods. Delete old implementation and write new. The only difference would be that in the second case you have to delete the qt implementation and in the first you have to replace self.oops() with the real implementation. What is the benefit then of having layer of wrapper classes? Is it the nice list of methods that must be implemented and methods that may be implemented? Well this list might be kept in the comments and copied from one GUI to another the same way it is now copied from wrapper classes. You may say that this architecture allows a string GUI that is used for testing, but this is another story. Many of the unit tests in their present implementation are useless because they are not testing the production code but some other code that is never used. Wherever the `if g.unitTesting` is used the code that is being tested and the code that is being executed in normal usage are different. To see for your self how little of production code is being tested I suggest you to delete some methods from qt classes or even to replace some objects with totally different classes and then execute unit tests. For many of changes you can make this way the result would be all unit tests passed. One of the promises unit test make is that overall code quality improves. The more testable code usually means more flexible and better code. The more pure functions in the code the more testable code it becomes. That is why functions should be preferred over classes which is not in Leo's code at the moment. I am not saying that we should use no classes at all. Some classes are necessary but many of Leo's classes are not necessary and they just make testing code harder than it needs to be. The interface between Leo's code is minimal. > This is another claim that I believe to be false. Before some recent changes I have made in this area, the outline drawing code was surprisingly spending more than 40% time in os.path.norm_path. Those are removed now but they are signs of deeper interconnection between the core and drawing code than you claimed to be. Drawing outline still has some calls to c.setCurrentPosition which really shouldn't be there If the principle of doing only one thing is followed. The presence of these calls in the drawing code to me is obviously wrong. I might not be able to explain it clearly enough. But I am sure if you dive into the problem with open mind you'll see that there can't be any logical explanation why these calls would be necessary inside the drawing code. From the present situation it is not clear which object is responsible for selecting and which is responsible for drawing. If the request for drawing is coming from commander then the commander should be responsible to call c.setCurrentPosition prior to calling tree.redraw() and tree.redraw should not call c.setCurrentPosition in any logical scenario. You might say that calling c.setCurrentPosition is benign but event if it is so, it adds more unnecessary complexity and prevents some other potential code simplifications. The c.hoistStack and c.chapterController are also deeply interconnected with the tree drawing and tree selecting. Of course they had to be, but perhaps c.hoistStack should not be in the core. Its purpose is totally gui related and it would be much better if this field was in the tree and not in the core commander. The core should not be concerned with how the tree is displayed but at present it is and it makes another bunch of interconnections between core and the view classes. All those unnecessary interconnections add to the complexity of the code. So the code is not as simple as possible but much more complex. It is easy to just dismiss all this facts and claim that complexity is unavoidable but that won't be true. I could add many more items on the list of necessary refactorings but it would be useless unless you accept the fact that the code is not rock solid as you claim it is and there is in fact a great need to revise the code. Fixing bugs and adding new features almost always leads to sub-optimal code and refactoring is necessary from time to time to keep code manageable. However you wrote: The present code is reasonable and rock solid. There is no great need to > revise it, and I have no plans to do so. > And what can I or anyone else say to that? For such large scale refactorings you are the only one who can approve them. I am willing to do the necessary work, but I am not prepared to spend hours and weeks on the refactorings if you are not willing to accept them in the end. We have to at least agree that some refactoring is necessary. There was a discussion on rewriting from scratch or using some other strategy for refactoring code. I have read about the mikado method as the good way to tackle refactoring of large projects. Basically it suggests to change any part of code and see what parts of the application are broken. Then revert the code in the initial state and make change somewhere else and record what is broken by this change. Using this technique one can learn quickly about hidden interconnections between modules. With this knowledge acquired it is usually possible to make some refactoring with greater confidence. When I read about this method I couldn't help but see the Leo as the perfect example of large project which needs to be refactored. In the past I have made several attempts to improve drawing code. Most recently I worked on the tree-refresh branch which avoids deleting tree items and uses a fast diffing algorithm to find which items should be changed. It gives some improvements but not enough to be proposed as the way to go. However I am sure that in combination with some other refactorings like moving hoisting logic and chapter logic from core to the view and cleaning the drawing code from the calls to tree.select and c.setCurrentPosition this approach can be very good. Actually both ideas using diff algorithm to change the tree and not to recreate it from the scratch and the idea of drawing only handful of nodes that user can actually see can't give enough performance gain unless these other refactorings are done. Once they are done the drawing won't be a problem any more. Both of these ideas can't really shine until these refactorings are done. There are many more possible improvements that depend on some of these large scale refactorings that can't be done without your approval. For example I've been working on the binary python extension written in rust and one written in cython. Both of them are very promising. I have tested them on both Linux and Windows. I can't test it on Mac because I don't have a Mac, but according to the cython and rust documentation there should be no problem compiling this extensions on Mac and they should work on Mac too. Using the extension written in rust I managed to load LeoPyRef.leo along with all at-file nodes in about 40-80ms. The tree has more than 8000 nodes. Traversing this tree using the binary extension takes less than 4ms. I haven't written yet the reading at-auto files, but this should not be too complicated either. I am quite confident that using this module for reading the whole tree and building vnodes from it to be used directly in the Leo as it is now could replace leoAtFile and leoFileCommands modules or at least functions from these modules could use binary extension if available to delegate their work to the binary extension and loading large outlines like LeoPyRef.leo would take about 200ms. The drawing code that would rely on the binary extension if available could make a great difference especially in the idea of using diff algorithm to just change the tree instead of building it from scratch every time c.redraw is called. Vitalije -- You received this message because you are subscribed to the Google Groups "leo-editor" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/leo-editor/12de9949-be98-4896-aa58-9d6f5dcbbd7b%40googlegroups.com.
