Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

Don't set IDE scripts that don't compile #1269

Merged

Conversation

montegoulding
Copy link
Member

Previously there was a check to ensure script editor
scripts were not set if they didn't compile. This has
been extended to all scripts when gREVDevelopment is
true.

This change makes it much simpler to edit
IDE scripts, however, the team needs to be aware that
scripts with compile errors will not save. If necessary
set gREVDevelopment to false temporarily to work around.

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

What will happen if users set this to true and then edit scripts? Currently non-compiling scripts can be saved as works in progress, and this is useful when editing someone else's long scripts. Will there be an indication that the script can not be saved or will this just be a silent failure? The latter would be a red flag for me.

@montegoulding
Copy link
Member Author

At the moment with this patch there will be a silent failure to set the actual object script although the script editor will highlight the errors so it will be rather obvious as long as people know that gREVDevelopment causes this behavior. This has been the case for the script editor scripts for nearly 5 years. It's worth noting that setting gREVDevelopment to true is only really intended for people working on the IDE. Note that there is another patch that forces some preferences based on gREVDevelopment.

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

Hmmm... maybe I've used glx2 for so long that I've forgotten how the IDE's editor works, but with glx2 it's certainly possible to save works in progress even if they don't compile. Ah! can you still save the script but not compile it? I'd be fine with that.

@montegoulding
Copy link
Member Author

I'm not sure what you mean there. The entire point is we dont want to set the script of the object to a script that doesn't compile because doing so removes all its handlers from the message path and this causes uncompiled critical IDE script hell syndrome

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

heh... yes, I know that syndrome. But saving the text is different from setting the script. And that's the way glx2 has always worked.

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

...and I see you put a notice in the use-list, so I'll move commenting there.

@montegoulding
Copy link
Member Author

OK, I don't know how glx2 works but the IDE has an intermediary object it uses to compile the script to check for errors and then it applies the script to the object itself. What this does is skip that last step if there's a compile error and gREVDevelopment is true. If we were implementing this for general use we would most definitely want some warnings at various points.

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

Yes, I think that's the only way to do this (compile, check for errors, set if possible). As long as there are annoying warnings and it's still possible to save the script text without setting it then I'm happy. I wouldn't want to spend a couple of hours working on a script, still not have it compile-ready, and not be able to save what I had done before going to bed. Working on someone else's 15k-line script (I've been there) gets to be an all-or-nothing marathon in that case.

But saving the script/stack and setting the script are two different things to talk about, so I think we're ok.

@runrevmark
Copy link
Contributor

This is certainly a neat idea for when you are editing certain IDE scripts . However, I tend to run with gREVDevelopment true when I'm not editing the IDE just so I can see if there are any IDE errors being thrown which I can report to get fixed. Also, with the patch which sets gREVDevelopment true (which I believe was recently merged) when building from source, there will be even more scenarios when (even us) will be running and using the IDE when we aren't actually editing it (i.e. pretty much me, all the time when doing engine dev).

I wonder if this extra 'bit of safety' should be tied to whether a stack is an IDE stack, rather than all stacks. It should also probably 'do something' in the script editor window when a script didn't compile and thus has not been set on an object so that you can visually see that if you close the script editor, you might lose work.

@mwieder
Copy link
Contributor

mwieder commented Jul 2, 2016

Yes, it's the "losing work" part that troubles me. I also run with gRevDevelopment true much of the time, partly because I'm working at a system level quite a bit, and also partly because it's enabled me to catch and submit fixes for some IDE errors that have previously gone unnoticed.

Previously there was a check to ensure script editor
scripts were not set if they didn't compile. This has
been extended to all IDE scripts.

An appropriate warning is displayed if the script
editor or tab will be closed offering the chance to
save uncompilable work in progress if necessary.
@montegoulding
Copy link
Member Author

OK, I've tweaked this a little so that:

  • the new behavior is only for IDE scripts
  • the same dialog that appears when closing an unapplied tab or the editor with unapplied scripts is presented
  • in the event that the user wants to save a work in progress on IDE scripts (there are many that won't break the IDE) then they can force apply from here due to pIgnoreErrors being true
  • because the status of the script is error rather than applied the icon on the tab is (and was in the previous commit) red and errors are displayed so it is clear that the script is not applied (green) or modified unapplied (yellow).

I actually think this new IDE behavior would work well in all cases. The only significant hurdle would be crashes while sorting out compile errors but we would like to avoid them regardless...

@montegoulding
Copy link
Member Author

The script editor save handling probably needs some sorting out as at the moment it will appear to save but the erroring script won’t be applied. I’m looking into that now. I think in the event the script application couldn’t happen it should beep at you or something rather than save.

@montegoulding
Copy link
Member Author

Hmm... Actually the save behavior is a bit odd. It applies all scripts but only saves the stack with the currently open tab. What if you didn't want some other edit saved? I guess it is a helper if you are eating multiple scripts from the same stack though. Perhaps it should save all stacks too though as it does give that appearance?

@montegoulding
Copy link
Member Author

Actually I misread the code and it only applies scripts from the stack of the currently being edited script which makes much more sense!

@mwieder
Copy link
Contributor

mwieder commented Jul 3, 2016

Thanks, @montegoulding this is sounding good to me now.

This patch ensures that the user receives a
warning when trying to save an IDE script
from the script editor and one of the open
scripts from that mainstack has compilation
errors.

It also replaces revTargetStack with
ideMainStackOfObject to ensure all
open scripts within a stackFile are
applied during a save. Previously
editing the script of a mainstack
object and the script of a substack
object would not apply them both
before save.
@montegoulding montegoulding changed the title Don't set scripts that don't compile if gREVDevelopment is true Don't set IDE scripts that don't compile Jul 5, 2016
@livecodeali
Copy link
Member

@livecode-vulcan review ok c238c51

@livecode-vulcan
Copy link

💙 review by @livecodeali ok c238c51

@montegoulding
Copy link
Member Author

Thanks @livecodeali once we have used this for a bit to confirm it covers all bases we can look at enabling this for everyone I think.

@montegoulding montegoulding merged commit 7c706bf into livecode:develop-8.0 Jul 11, 2016
@montegoulding montegoulding deleted the dontsetuncompiledscripts branch July 11, 2016 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants