Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Async exec refactoring #815

Closed
wants to merge 2 commits into from
Closed

Async exec refactoring #815

wants to merge 2 commits into from

Conversation

arichiardi
Copy link
Contributor

Hello Laurent,
I was really really bothered by the tests not succeeding and therefore I refactored all the .asyncExec functions in one place (DisplayUtil) so that we can check for null and isDisposed before executing any asyncExec/syncExec code on Display.

I hope I did not break anything and I still need to check that all the functionalities are working (especially the Outline). In the meantime I am putting this PR up so that I can check if Travis has green tests like me on my machine.

I also would like to ask you some question in the code that I refactored.

👍

@arichiardi
Copy link
Contributor Author

Sorry for the current mess, I have not yet rebased on master, I will do it.

@arichiardi arichiardi force-pushed the async-exec branch 5 times, most recently from 121e29a to 2241f82 Compare July 13, 2015 17:27
@arichiardi
Copy link
Contributor Author

Great this is green again!
I would like to add the SWTBot test generator as part of the menu, like in here under Programatically, from a plugin, only of course maybe if ccw is launched with -debug. What do you think?
And maybe while testing this addition I will also add some tests to SmokeTests 😄

@arichiardi
Copy link
Contributor Author

Please disregard the first two commits, I am going to remove them as they are not part of this PR.

@arichiardi arichiardi force-pushed the async-exec branch 2 times, most recently from e2f2de5 to 3a21ccc Compare July 13, 2015 19:12
@arichiardi
refactor(asyncExec): all asyncExec/syncExec is now in DisplayUtil and…
1e3819e 
… everything uses it

In order to have a single point of failure, all the calls to display.asyncExec and display.syncExec
are now pointing to the DisplayUtil class.  This required some refactoring of both .java and .clj
files: in particular some code that originally was in eclipse.clj now has been moved to
swt.clj (there was a TODO about it).
@arichiardi
fix(ccw.core.test): SmokeTests are green again
afe5ddb 
Some change was necessary in order for the tests to work again. Together with it, ClojureEditorTests
has been removed as it seemed duplicated code, residual of some test.
@arichiardi
Copy link
Contributor Author

This one should be ready, but I would like to merge the swtbot-generator branch first so that I can work on some tests here...ok?

@laurentpetit
Copy link
Member

Ok but please not in master. I'd like to prepare a release and stabilize things ! :-)

—
Envoyé avec Mailbox

On Fri, Jul 17, 2015 at 7:16 PM, Andrea Richiardi
notifications@github.com wrote:

This one should be ready, but I would like to merge the swtbot-generator branch first so that I can work on some tests here...ok?

Reply to this email directly or view it on GitHub:
#815 (comment)

@arichiardi
Copy link
Contributor Author

Sure thing! Of course I will be waiting your approval 😉

@arichiardi
Copy link
Contributor Author

Hello Laurent, what about this PR, have you had time to have a look at it? It would also solve the SmokeTests issue and we would have all green again..

@laurentpetit laurentpetit added this to the 0.32.0 milestone Jul 23, 2015
@laurentpetit
Copy link
Member

Sure I need to review it, it was not on my radar, sorry

@arichiardi
Copy link
Contributor Author

👍 Thanks, just because I was taking up coding the partitions and I noticed this (that I refactored out from there) was not in yet... 😄

laurentpetit
laurentpetit reviewed Jul 23, 2015
View reviewed changes
ccw.core/src/clj/ccw/editors/clojure/hover_support.clj
@@ -352,8 +352,8 @@
"Updates the observable-hovers part of the state. Returns new state."
[old-state descriptors]
(let [observable-hovers ^ObservableList (:observable-descriptors old-state)
realm (SWTObservables/getRealm (workbench-display))]
(.exec realm #(do (.clear observable-hovers)
realm (SWTObservables/getRealm (swt/display))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering whether we should keep workbench-display and swt/display ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prolly not 😄

@laurentpetit
Copy link
Member

If you don't mind, I will first test commit 1e3819e and add (after some bed time) the additional modifications I brainstormed on the comments.

I don't understand everything in the second commit (related to SmokeTests), but this can be done as a separate work not preventing me from integrating the work on ui.

@laurentpetit
Copy link
Member

we'll keep the PR around for the time being, as it includes interesting comments/discussions

@laurentpetit laurentpetit mentioned this pull request Jul 23, 2015
Merged
@arichiardi
Copy link
Contributor Author

Ok I saw the other PR, I will probably just integrate the second commit in swtbot-generator, the one that is more about tests...which BTW can be merged as far as it is all green 👍
I am planning to reintroduce your mistakenly removed test there as well don't worry 😄

@laurentpetit
Copy link
Member

(y) ok. So as soon as I've done a new pass across the comments on this PR, I will close it.

@arichiardi arichiardi deleted the async-exec branch July 26, 2015 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
To Review
Projects
None yet
Milestone
0.32.0
Development

Successfully merging this pull request may close these issues.
2 participants
@arichiardi @laurentpetit