VeLaObSource: code editor undo/redo; new "Check" button: check code and parameters before execution

[mpyat2]

see #442

[dbenn]

Looks good @mpyat2. Thanks!

This works well as a syntax check. A refinement would be to check that the function has a real parameter and return type. This will pass the check for example:

f(t:real) { t^2 }

However, the error will still be picked up when the OK button is clicked.

I have a question. undo/redo is mentioned in the title of this issue. Is that intended to be undo/redo in the Edit menu? I don't see those menu items change (on my Mac at least). I see that ctrl-Z / cmd-Z works for undo, which is cool. I'm not sure what key redo is bound to.

[mpyat2]

Hi @dbenn
thank you for the suggestion about the parameter check. I'll try.

About undo/redo: well, I did not mention it anywhere; actually, I've implemented CTRL+Z / CTRL-Y keystrokes for the undo and redo operations respectively, not a menu commands; I suppose on Mac it should be COMMAND+Z / COMMAND+Y.

[dbenn]

Thanks @mpyat2. Yep, command+Y on Mac works. I was initially thinking you were using VStar's UndoableEditManager, hence the confusion. May be able to make a link in future, but no worries for now.

Re: parameter, return type checking, have a look at VeLaModelCreator.execute() where we have:

// Has a model function been defined?
if (!vela.lookupFunctions(FUNC_NAME)
		.isPresent()) {
	MessageBox.showErrorDialog(
			"VeLa Model Error",
			"f(t:real):real undefined");

If .isPresent() is true, you can go further -- which I have not here but should! -- and call .get() to get the FunctionExecutor then call functions on it to get formal parameter list and return type.

Having said all that, if you want to just wait for it to be picked up at run time, that's fair enough too.

[dbenn]

@mpyat2 on an unrelated note, I noticed that this dialog's "Add to Current?" checkbox defaults to checked, unlike all other observation source dialogs (as far as I know).

[mpyat2]

Hi @dbenn ,
I completely forgot (if I ever knew) about UndoableEditManager. Well, I'll try to investigate; however, it's better next time.

I've implemented the stricter check for the model function definition. Probably too straight and not elegant.

What I've noticed:
if we have more than one f() function with identical parameters, the parser uses the last one and ignores all the previous ones (if we have several overloaded functions, everything is correct). Probably, it's better to throw an error for this. For example:

zeroPoint is 2457504.93 # time zero point
magZeroPoint is 13.69
period is 0.0850674

f(t: real): real {
   magZeroPoint
   + 0.091481685488957 * cos(2*PI*(1/period)*(t-zeroPoint)) + 0.114900355450183 * sin(2*PI*(1/period)*(t-zeroPoint))
   - 0.031986371275697 * cos(2*PI*(2/period)*(t-zeroPoint)) - 0.029782272061918 * sin(2*PI*(2/period)*(t-zeroPoint))
   - 0.005402185898561 * cos(2*PI*(3/period)*(t-zeroPoint)) + 0.001484256405225 * sin(2*PI*(3/period)*(t-zeroPoint))
   + 0.006091217702922 * cos(2*PI*(4/period)*(t-zeroPoint)) + 0.001654399074451 * sin(2*PI*(4/period)*(t-zeroPoint))
   - 0.004698206584795 * cos(2*PI*(5/period)*(t-zeroPoint)) - 0.000039671630067 * sin(2*PI*(4/period)*(t-zeroPoint))
   + 0.003549883073703 * cos(2*PI*(6/period)*(t-zeroPoint)) + 0.000022578051393 * sin(2*PI*(6/period)*(t-zeroPoint))
}

f(t:real):real {
  magZeroPoint
}

The second (short) function will be used, the first will be ignored.

About "Add to Current?" which is checked by default: the observation source is something between the 'true' obs. source and the model. I thought that it would be used to plot a 'continuous' model over the data.

If you think it would be better to uncheck the "Add to Current?" checkbox by default (for consistency with other sources), I'll do it.

[dbenn]

Thanks for all this @mpyat2.

No worries re: undo/redo.

I agree with what you say re: "Add to Current?" so leave as you have it now.

Interesting question about functions being redefined. A philosophical language design question. I have taken the same approach as some languages, e.g. Python, in which a function can be redefined:

>>> def f(n):
...   return n
... 
>>> f(2)
2
>>> def f(n):
...   return n*n
... 
>>> f(2)
4

That doesn't mean I agree with all of Python's design choices. Definitely not. Some languages allow this, some don't, e.g. Java does not. It's a good question. I think I'd like to spend some time considering it, weighing up the pros and cons as part of a separate issue, and not to delay merging the current work. Is that OK with you?

[mpyat2]

Hi @dbenn ,
Yes, surely, you are the creator of VeLa, so the final word is yours!
No problem at all!
(and it does not interfere with the current changes)

[mpyat2]

Thank you, @dbenn !

[dbenn]

Thank YOU @mpyat2! :)