- User Since
- Dec 31 2015, 9:47 AM (120 w, 6 d)
I cannot reproduce this with my third patch. Can you paste here your debug output for this example?
The problem was in LuaSession::readOutput(). The patch
handles the outputs correctly. @sirgienko Does it also work for you?
Mon, Apr 23
Sun, Apr 22
Yes, it works. But the implementation seems to be a bit complicated. Wouldn't be easier to simply split the output of lua with QString::split('\n') and QString::split('>') and to remove the input commands from the obtained QStringList?
@sirgienko I think you have already a KDE account, right? So, you can push the patches after the code review. Thanks.
Sat, Apr 21
Thu, Apr 19
Tue, Apr 17
Mon, Apr 16
Sun, Apr 15
Sat, Apr 14
Wed, Apr 11
Sorry, I over looked the highlighted commas. Your patch is ok.
You showed already a lot of interest in Cantor and good understanding of the code and architecture. If you plan to further contribute here it makes sense to apply for a KDE account. But let's maybe still go via the review process at the beginning. Don't push directly without a review. Even small fixes can cause big troubles. Doing review for small patches is ok for me, don't worry about this.
I don't exactly understand the problem you're trying to fix. Octave session for me without your patch:
What exactly is wrong here?
Mon, Apr 9
Sun, Apr 8
Your diff seems to be wrong. Looks like you calculated the diff between the current version and the temporary version you had before with m_supressRuleChangedSignal. Can you please upload the corrected diff?
Sat, Apr 7
I agree, that we should simplify this templates methods to not template functions, which acepting QStringList. Should I add it to this diff?
Yes, please. Let's do the clean-up here in the same diff.
Can we try to implement this with a boolean paramer m_suppressRuleChangedSignal instead of adding new functions? In addRules() we set m_suppressRuleChangedSignal = true and use in addRule()
Submitted in a0597aedc989ccc67dd6a2c39f32767df766a671.
Fri, Apr 6
About the add_definitions you should not need it, the generated
already has that define, just make sure you include that file wherever you need it.
I don't see any includes for config-cantrolib.h in the code. Maybe this is added somewhere on the cmake-level. Some parts of the current code are aware of the definition of WITH_EPS, e.g. Expression::setResult(). Some other parts like Worksheet:: loginToSession() don't "see" this variable and require now either that include you mentioned or a preprocessor definition. Since #ifdef WITH_EPS is used in several different places in Cantor's code it will take some time now to test all those places. Given the current timeline/deadline for 18.04 I'd rather go with a preprocessor definition via add_defitions() - this is safer now and also more clean and clear, at least to me, since it's straight -forward to see where it's defined.
This is really strange. I don't see nowhere in cantor/cmake/FindLibSpectre.cmake (not sure this file is really used by cmake) the variable LibSpectre_FOUND. Since cmake's variables are case sensitive I'd assume LIBSPECTRE_FOUND is the only correct one. Your cmake is newer than the version that I'm using. But this worked in cantor already in the past for sure, also with older versions of cmake...
Thu, Apr 5
What about octave warnings? Are they written out to stderr by octave?
I'd rather call appendEntry(CommandEntry::Type), etc. directly in Worksheet::load(QIODevice*) instead of going via all those append*Entry() functions - this additional indirection (one more function call)
I am not sure, that compiler don't optimize it in inline call.
The compiler will most probably inline those calls, yes.
@asemke, I also found problem with c3aa8b3e18d4c7be8c094a5acbd4864b09b5eb02: If we choose octave backend, but don't write anything (so login don't called) and trying to open saved octave worksheet, we get segfault, because in this case we never call OctaveSession::login (i have checked it with gdb).
This is fixed now with 2ca024892335728f48d7aa3dfc22032b39b10819. Thanks for reporting this.
Tue, Apr 3
thanks for this finding. The login in Worksheet::load() was redundant. I removed it in c3aa8b3e18d4c7be8c094a5acbd4864b09b5eb02. Can you please also check it on your side?
Oh, sorry, I was looking for cantor_plot* and not for cantor_print. Now I've got it. Your fix makes sense. But add those contains-calls into the if-statement so we don't need to call QString::contains() twice for nothing in case m_plotPending is false. Simply use