SketchException refactor - #1164
Conversation
Sorry, something went wrong.
This reverts commit 105bc96.
fixes bug with selecting an error that's in a tab that isn't visible yet.
…dd-aj-m docs: add aj-m as a contributor for code
…dd-pnngocdoan docs: add pnngocdoan as a contributor for code
…dd-manoellribeiro docs: add manoellribeiro as a contributor for doc
…or-guidelines-assignment
Improved the phrasing on Jetpack Compose
…asing-jetpack Update BUILD.md
Stefterv
left a comment
There was a problem hiding this comment.
Hi @joshgiesbrecht Sorry it took me so long to take a good look at this. Thank you for taking this on and fixing this issue!
Looks good to me, I added a few comments that are mostly about style.
Sorry, something went wrong.
| id("java") | ||
| } | ||
|
|
||
| group = "org.example" |
There was a problem hiding this comment.
Please remove these
Sorry, something went wrong.
There was a problem hiding this comment.
by "these" you meant the group and version lines, right?
Sorry, something went wrong.
There was a problem hiding this comment.
Yep
Sorry, something went wrong.
| @@ -1,2 +1,3 @@ | |||
| bin | |||
| pde.jar | |||
| /utils/build/ | |||
There was a problem hiding this comment.
Please use the top level .gitignore
Sorry, something went wrong.
…ls.SketchException, except I'm missing something because on compile it's not finding it. (module / package visibility problem or somethin')
…s for now though(?)
…ketchException-refactor # Conflicts: # app/.gitignore # app/utils/build.gradle.kts
…s for now though(?)
…ketchException-refactor
|
Ok, my git skills are weak, and I can't figure out why this PR is dumping so many excess commits in after I rebased the actual changes to an up-to-date main. The branch on my fork of the repo looks reasonable: main...joshgiesbrecht:processing4:SketchException-refactor Should I resubmit the PR? Or do you know of some way to make it smarten up? (I'm also not sure how this has ended up failing tests, which is not a great sign. It's building here when I checkout the refactor branch.) |
Sorry, something went wrong.
|
My git knowledge is also not good enough to tell you exactly what you need to do. Feel free to cherry pick your changes onto a new branch and open a new PR. As for the failing tests, I checked and it makes sense that they are failing, the SketchException class is taken out of a context that Ant (our legacy build system) has access to. One of our next steps is to do away with the Ant based build system. For now I could start disabling the Ant based workflows, what do you think @SableRaf ? |
Sorry, something went wrong.
That sounds reasonable :) |
Sorry, something went wrong.
Created an :app:utils module, moved SketchException to a processing.utils package within :app:utils, changed all references to SketchException in both java and app to use processing.utils.SketchException, removed the two duplicate SketchException classes in app and java.preproc.
This currently seems to work (tested only on Linux so far, but I don't think there's anything platform-specific involved), and does solve the bug of highlighting the first error on running a sketch. Non-Java modes will not yet be using the correct SketchException class; I suspect this will be a 'soft' fail that has a similar problem to the existing bug, but I haven't tested this much yet. (Shader mode seems to be broken already?)
Since the draft PR for #1104 also creates :app:utils and is more complex than this PR, I'm fine with waiting until the other PR is merged, and then I can make sure this one merges cleanly before it's added in.
What's the right thing to do re: the impact on non-Java modes? Should I submit PRs for those projects as well, maybe once this one's actually merged into main?