New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the change detector not reload the sketch #5021

Merged
merged 2 commits into from May 19, 2017

Conversation

Projects
None yet
3 participants
@GKFX
Contributor

GKFX commented Apr 22, 2017

Fixes #4713, fixes #4849. I've added a method to Messages that accepts custom buttons, and Messages.showYesNoQuestion is now based on it for Mac OS.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Apr 22, 2017

Member

@JakubValtar have a moment to review?

Member

benfry commented Apr 22, 2017

@JakubValtar have a moment to review?

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Apr 23, 2017

Contributor

@benfry I'm travelling, but I should have time today evening.

Contributor

JakubValtar commented Apr 23, 2017

@benfry I'm travelling, but I should have time today evening.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Apr 23, 2017

Member

Thanks!

Member

benfry commented Apr 23, 2017

Thanks!

@JakubValtar

Great job @GKFX, this looks really good! I added some comments into the code.

There are two more things which should be addressed (I can do it after the merge so we don't complicate this PR): error checker does not kick in after code is added or reloaded. The other thing is that we should run the whole check on the EDT so that the UI is blocked until the check is done and user does not get a chance to mess with the code before the check finishes.

Show outdated Hide outdated app/src/processing/app/ui/ChangeDetector.java
Show outdated Hide outdated app/src/processing/app/ui/ChangeDetector.java
Show outdated Hide outdated app/src/processing/app/ui/ChangeDetector.java
# External changes detector
change_detect.reload.title=Tab modified externally
change_detect.reload.question="%s" was modified by another program.
change_detect.reload.comment=Would you like to keep this version or load the new changes?\nEither way, the version you discard will be saved to your sketch folder.

This comment has been minimized.

@JakubValtar

JakubValtar May 4, 2017

Contributor

@benfry I know you proposed this message before, but I think it's confusing.

image

Something along these lines seems more clear:

image

@JakubValtar

JakubValtar May 4, 2017

Contributor

@benfry I know you proposed this message before, but I think it's confusing.

image

Something along these lines seems more clear:

image

This comment has been minimized.

@benfry

benfry May 5, 2017

Member

The language needs to be rewritten in the first, but it's mostly in the direction we need to go. I think dropping the final sentence would get rid of most of the awkwardness. It could even be shown in a second dialog box (or just in the console as a message).

Your suggested version works great for people already familiar with IDEs or advanced text editors that have this kind of feature. For the majority of our audience, it's not clear what "reload" really means in this context: do I lose my work? Does it save something? What's happening?

@benfry

benfry May 5, 2017

Member

The language needs to be rewritten in the first, but it's mostly in the direction we need to go. I think dropping the final sentence would get rid of most of the awkwardness. It could even be shown in a second dialog box (or just in the console as a message).

Your suggested version works great for people already familiar with IDEs or advanced text editors that have this kind of feature. For the majority of our audience, it's not clear what "reload" really means in this context: do I lose my work? Does it save something? What's happening?

This comment has been minimized.

@JakubValtar

JakubValtar May 5, 2017

Contributor

Ok, understood. I think my problem was with 'keep this version'. What is a version? Does this refer to the externally modified version from the previous sentence?

Something like 'Do you want to keep what is in the test5 tab or reload the tab from the file?' may be better?

Just a proposal, it's your call.

@JakubValtar

JakubValtar May 5, 2017

Contributor

Ok, understood. I think my problem was with 'keep this version'. What is a version? Does this refer to the externally modified version from the previous sentence?

Something like 'Do you want to keep what is in the test5 tab or reload the tab from the file?' may be better?

Just a proposal, it's your call.

This comment has been minimized.

@benfry

benfry May 5, 2017

Member

Right, I think it needs to be rewritten, “this version” is confusing. (But I can take care of that separately from the code…)

@benfry

benfry May 5, 2017

Member

Right, I think it needs to be rewritten, “this version” is confusing. (But I can take care of that separately from the code…)

Show outdated Hide outdated app/src/processing/app/ui/ChangeDetector.java
Change Detector: fix typos and don't save on keep
UI text has not been updated
@GKFX

This comment has been minimized.

Show comment
Hide comment
@GKFX

GKFX May 11, 2017

Contributor

Okay, I've made the changes. That's excluding the UI text, since it makes more sense for you guys to tweak that, so change_detect.reload.comment is now inaccurate.

Contributor

GKFX commented May 11, 2017

Okay, I've made the changes. That's excluding the UI text, since it makes more sense for you guys to tweak that, so change_detect.reload.comment is now inaccurate.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 19, 2017

Contributor

@GKFX Thanks, looks great!

@benfry It's merge time! When it's done I'll push some changes on top (run it on EDT and nudge the error checker).

Contributor

JakubValtar commented May 19, 2017

@GKFX Thanks, looks great!

@benfry It's merge time! When it's done I'll push some changes on top (run it on EDT and nudge the error checker).

@benfry benfry merged commit 6895a29 into processing:master May 19, 2017

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 19, 2017

Member

Great, thanks guys.

Member

benfry commented May 19, 2017

Great, thanks guys.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 19, 2017

Contributor

@benfry Follow-up here: #5075

Contributor

JakubValtar commented May 19, 2017

@benfry Follow-up here: #5075

@GKFX GKFX deleted the GKFX:feature-safechangedetect2 branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment