Edit: the approach I took in this post to try to fix the issue was in error. See the follow-up post for an explanation of why it was incorrect, and for my description of the actual fix.
Since wxMaxima's describes itself as a "document-based interface for...Maxima", it would stand to reason that any actions within wxMaxima that would affect how a document will appear should register as changes. wxMaxima supports a section hierarchy and allows folding of sections, and while the state of the fold is saved in .wxm and .wxmx documents saved from wxMaxima, changing the state of a fold alone does not register as a change which will allow the file to be saved again.
wxMaxima uses a familiar "Save" / "Save As..." scheme. You can "Save" your document by pressing Ctrl+S at any time the document has unsaved changes. You can "Save As..." at any time, but it's less convenient to do so, especially if you plan to overwrite the file. Luckily "Save As..." gives a workaround for the problem.
Since it seems like a relatively simple user-interface improvement which will improve the intuitiveness of the document composition system that wxMaxima offers, I decided to take on this issue.
Investigation
In order to fix a bug, first you need to find the code that is responsible for the incorrect behavior. It is often easier to locate a bug, however, than to find the code that should be responsible for a feature that doesn't exist yet, but it is a similar process. This issue appears at face value to be a bug, but depending on how the "unsaved document" functionality was implemented, the folding functionality might simply be missing rather than broken, so we might be looking for where and how to add a feature on this one.
In a graphical program it can be somewhat challenging to locate where a feature should be included, especially when that feature may touch non-graphical (backend) portions of the program.
In wxMaxima, cells that can be folded have a square button displayed on their left side that you click to toggle the fold. Since I knew that the functionality I want to add has to do with folding, I did a quick <code>git grep "Fold"</code> and found that there are some functions related to folding in the GroupCell class. At this point, how the folding button actually works doesn't matter, because we have located the code that is processed when a fold occurs.
I also needed to find the code that sets the asterisk next to the filename when a file has been modified. <code>git grep "*"</code> found me a lot of pointers but also found me a line which set the "*" in the window title. I saw that the function to set the title in wxWidgets is called SetTitle, so I searched for that function name to make sure that the window title isn't set anywhere else.
Looks good. Now I need to find out how the program decides whether to include the * in the title. Looking at the function wxMaxima::ResetTitle, I notice a few things that look concerning. First, there's a lot of conditional compilation in this method, relating to the platform on which the program is compiled. In this case, a special case is made for Mac, probably because setting window titles in OS X works considerably differently or follows a different paradigm than the other platforms.
At this point, I got a little discouraged, but I decided to consider the parts that I could understand right away and I realized it's not all that complicated for my purposes. What we know is that the title is only set from here, and the logic for doing so is already completed. So we need to look at the control logic. If a variable, m_fileSaved is false, then the asterisk is added to the title. That variable is set in this function from the variable passed into the function. So we need to look at the callers of this function to find out where that value is coming from. We need to set that value to false when we have a fold event.
This looks promising, because ResetTitle() is not called from many places. wxMaxima::OnIdle, which says in the comments that this is where we check if the document is saved, and otherwise it is called only when documents are loaded, which we're not interested in right now. So looking at OnIdle(), we can see that the value is coming from m_console, which is a MathCtrl, and the method IsSaved() returns the value of MathCtrl::m_saved. We can set this value with MathCtrl::SetSaved(), but other than that, this seems like a dead end. We will need access to the MathCtrl which represents the current document when a Fold event occurs so that we can call SetSaved(), which will result in the window title being set.
Solution
Back in GroupCell.h, I note that there is not a reference to a MathCtrl in GroupCell. Nor is there a reference to a MathCtrl in GroupCell's parent class, MathCell. I looked around for a bit but since any kind of fold action eventually comes down to GroupCell, the easiest way to make sure that all folds will update the saved status is to update from GroupCell. That means that we have to add a MathCtrl reference to GroupCell, which means changing the GroupCell constructor, and that's a little scary.
Remember that even if the maintainer of the project objects to your change, you can always revert and go about it a different way. To make merging easier, I make a branch called "issue233" (rather than working in "master") and get to work.
I added m_mathCtrl to GroupCell.h, and add a function called FoldEvent() which I will call from every Fold-related function to let the MathCtrl know that a fold has occurred. I update the constructor to take a MathCtrl* and update m_mathCtrl. I included "MathCtrl.h" in the files which now newly use the MathCtrl type.
I use the call hierarchy to find a few locations where GroupCells are constructed and update those constructors to take a reference to a MathCtrl, and then try to compile.
Debugging
And right away, I got this error message:
g++ -DHAVE_CONFIG_H -I. -DPREFIX=\"/usr/local\" -g -O0 -I/usr/local/lib/wx/include/gtk2-ansi-release-2.8 -I/usr/local/include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXGTK__ -pthread -MT main.o -MD -MP -MF .deps/main.Tpo -c -o main.o main.cpp
In file included from MathCtrl.h:29,
from wxMaximaFrame.h:32,
from wxMaxima.h:23,
from main.cpp:32:
GroupCell.h:44: error: ‘MathCtrl’ has not been declared
GroupCell.h:112: error: ISO C++ forbids declaration of ‘MathCtrl’ with no type
GroupCell.h:112: error: expected ‘;’ before ‘*’ token
make[1]: *** [main.o] Error 1
make[1]: Leaving directory `/home/doug/dev/wxmaxima/src'
make: *** [all] Error 2
This is frustrating because I know I included the file. What's actually going on here is circular inclusion. MathCtrl includes GroupCell in order to work properly, but now GroupCell needs MathCtrl in order to work. The solution is, unfortunately, to just declare "class MathCtrl;" and let the linker figure it out, but this is feeling more and more like I might be taking the wrong approach. Might as well follow through with it now.
After changing to the forward declaration, I have the following error:
g++ -DHAVE_CONFIG_H -I. -DPREFIX=\"/usr/local\" -g -O0 -I/usr/local/lib/wx/include/gtk2-ansi-release-2.8 -I/usr/local/include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXGTK__ -pthread -MT GroupCell.o -MD -MP -MF .deps/GroupCell.Tpo -c -o GroupCell.o GroupCell.cpp
GroupCell.cpp: In member function ‘void GroupCell::FoldEvent()’:
GroupCell.cpp:1169: error: invalid use of incomplete type ‘struct MathCtrl’
GroupCell.h:40: error: forward declaration of ‘struct MathCtrl’
make[1]: *** [GroupCell.o] Error 1
make[1]: Leaving directory `/home/doug/dev/wxmaxima/src'
make: *** [all] Error 2
Apparently the problem is that because of the include order, the class has actually been included already before the forward declaration, so I ended up with this terribly messy solution:
#ifndef _MATHCTRL_H_
#include "MathCtrl.h"
#else
class MathCtrl; // provide class declaration because of circular inclusion
#endif
So this way, if the class hasn't already been included, we will include it, and otherwise, we will have the forward declaration.
Unfortunately while compiling that file worked just fine after that point, I broke the compilation of other files. This is looking none too promising. Luckily, it's a learning experience, and we're seeing that sometimes the first solution that occurs to us is not always the best. Tomorrow, I'll try to approach the folding from the possibly messier but less invasive method of dealing with folding from the MathCtrl class.
No comments:
Post a Comment