Overhauled the formatting and designs. #152

jrlanglois · 2020-12-08T20:33:07Z

There are many changes here

Stylistically changed many explicit variable types to use auto.
Sprinkled const-ref here and there, where necessary.
Flipped the wacky Yoda conditions.
Added a .gitattributes to make things consistent for cross-platform development.
Fixed up how third-party warnings are managed by adding a .h and .cpp to start/finish or push/pop ignoring warnings.
Lots of macro changes to use what JUCE offers, instead of the hand-rolled shenanigans.
Updated the .gitignore so the repo can cope with vscode and MSVC stuff.
Fixed Duktape's C++11 detection for MSVC.
Made Doxygen comments follow JUCE's conventions, and also reformatted them here and there.
Fixed up a lot of the C++11 (and up) std::function stuff to make it more readable.
Added more exporters to the unit test project.
Added a JUCE module compatible macro to be able to enable/disable unit tests from the Projucer and its Cmake equivalents.
Tried to avoid implicit bool and pointer usage (you'll see explicit != nullptr and == nullptr comparisons now).
Swapped typedef usage for using.
Removed all inclusions within the module and moved them into just the master module files.

Still in progress

Migrating implicit casts of juce::var to using static_cast<Type> so as to avoid any compiler faffery.
Need to fix some errors in the unit tests; errors that have cropped up after my slew of changes.

Why are you doing this?! ARE YOU MAD?!

Yes... Probably?

So, my approach here was to organically run through and do various passes to dig up any and all of the things that are inconsistent, potentially broken, missing, and whatever else qualifier. Some of the code is just plain crusty...

And no worries - I get that some contributors are just having fun, and the overall goal is to make something useful. Also, I'm aware that not everybody knows JUCE super well. Really I'm just pitching in to look at all of this from a framework development POV.

And yeah, this PR can be broken down into more digestable chunks! Treat this as a proof of concept and to gather ideas for what can come next.

jrlanglois · 2020-12-08T20:37:47Z

@nick-thompson Have a gander...

jrlanglois · 2020-12-08T20:43:53Z

blueprint/blueprint.cpp

- * resolve this.
- */
-#if defined (_WIN32) || defined (_WIN64)
-#define _WINSOCKAPI_


@nick-thompson This is no longer necessary now that I applied JUCE_CORE_INCLUDE_NATIVE_HEADERS above.

Nice. That one was annoying.

jrlanglois · 2020-12-08T20:44:23Z

blueprint/blueprint.h

 #include <juce_gui_basics/juce_gui_basics.h>

 #if JUCE_MODULE_AVAILABLE_juce_audio_processors
    #include <juce_audio_processors/juce_audio_processors.h>
 #endif

-#include "yoga/yoga/YGMacros.h"
+//==============================================================================
+#if JUCE_EXCEPTIONS_DISABLED


@nick-thompson Here are some checks to be sure your module works as expected and doesn't surprise the user with anything.

jrlanglois · 2020-12-08T20:44:34Z

blueprint/blueprint.h

+#endif
+
+//==============================================================================
+/** Config: BLUEPRINT_INCLUDE_UNIT_TESTS


The Projucer compatible config/macro.

jrlanglois · 2020-12-08T20:44:50Z

blueprint/blueprint.h

+    /** This is needed because Duktape does a faulty check for MSVC toolchains,
+        specifically because MSVC has NEVER supported the __cplusplus macro.
+    */
+    #define DUK_F_CPP11 (JUCE_CXX14_IS_AVAILABLE)


Fixing Duktape's C++11 detection here...

jrlanglois · 2020-12-08T20:45:47Z

blueprint/core/blueprint_AppHarness.cpp

@@ -1,85 +1,79 @@
-/*


Notice I removed these comments as they don't provide any value whatsoever (a git log or visit to GitHub will show any and all of this).

Realistcally, a top-of-the-file comment header license thing should be applied to all of your module's code files at some point.

They really bugged me! I think thats an Xcode or Projucer generation thing ...

Yeah, it definitely is.

jrlanglois · 2020-12-08T20:46:16Z

blueprint/core/blueprint_EcmascriptEngine.cpp

        static void fatalErrorHandler (void* udata, const char* msg)
        {
-            (void) udata; // Ignored in this case, silence warning
+            juce::ignoreUnused (udata);


JUCE provides this func to ignore params... Pretty useful stuff

jrlanglois · 2020-12-08T20:46:46Z

blueprint/core/blueprint_EcmascriptEngine.cpp

@@ -95,9 +82,12 @@ namespace blueprint
        jassert(code.isNotEmpty());
        auto* ctxRawPtr = dukContext.get();

-        try {
+        try
+        {


Either we're doing braces one way or the other way... The mix of braces is downright gastly, especially in a public framework.

Much prefer this style.

jrlanglois · 2020-12-08T20:47:34Z

blueprint/core/blueprint_FileWatcher.h

@@ -60,12 +70,11 @@ namespace blueprint
            // may delete the target file just before replacing it with their
            // new compiled result, and if we happen to poll in between we'll
            // get a messed up result.
-            for (auto it = watchedFiles.begin(); it != watchedFiles.end();)
+            for (auto it = watchedFiles.begin(); it != watchedFiles.end(); ++it)


Fixed some weirdness here...

jrlanglois · 2020-12-08T20:48:09Z

blueprint/core/blueprint_RawTextView.h

-        {
-            throw std::logic_error("A RawTextView can't receive properties.");
-        }
+        /** */


Lots of Doxygen needs to be filled out.

jrlanglois · 2020-12-08T20:48:56Z

blueprint/core/blueprint_ReactApplicationRoot.cpp

-        DBG(err.context);
-        DBG("");
-        DBG(err.what());
+        juce::Logger::writeToLog ("\n==== Error in JavaScript runtime. Context: ====\n" + err.context + juce::newLine + err.what());


Using DBG isn't great from a user's point of view, and in fact it might not even be helpful in a release build. So, I swapped these bits out for a log.

jrlanglois · 2020-12-08T20:50:00Z

blueprint/core/blueprint_ReactApplicationRoot.h

+        causing the application to suspend execution and await connection from a debug client.
+        See the documentation for details on setting up and connecting a debugger.
+    */
+    class ReactApplicationRoot final : public View


I've also gone and marked nearly everything public with final. I'm rather doubtful of the necessity for users to override these super complex classes; it would really be a special case worth discussing with the user imho.

jrlanglois · 2020-12-08T20:50:31Z

blueprint/core/blueprint_ReactApplicationRoot.h

    private:
        //==============================================================================
-        template <int NumParams, typename MethodType>
+        ViewManager viewManager;


Tried to keep members first / methods next in the private section. Doing that is much easier to visually scan.

jrlanglois · 2020-12-08T20:51:04Z

blueprint/core/blueprint_ShadowView.cpp

-        {YGEdgeToString(YGEdgeBottom), getYogaNodeDimensionSetter(BP_SPREAD_SETTER_PERCENT(YGNodeStyleSetPosition), YGEdgeBottom)},
-    };
+        static const PropertySetterMap propertySetters{
+            {"direction", getYogaNodeEnumSetter(YGNodeStyleSetDirection, ValidDirectionValues)},


This area is also in need of some TLC whitespace... It's pretty hard on the eyes!

jrlanglois · 2020-12-08T20:52:09Z

blueprint/core/blueprint_ShadowView.h

    class ShadowView
    {
    public:
        //==============================================================================
-        ShadowView(View* _view) : view(_view)
+        /** */
+        ShadowView(View* v) :


This is just my POV but seeing that we have a .cpp file going, it might be better to move ShadowView's method bodies into it... This header is also a lot to digest.

There's a view headers that might benefit from being split now.

jrlanglois · 2020-12-08T20:52:52Z

blueprint/tests/blueprint_EcmascriptEngineTests.cpp

-        expect (5 == (int) engine.evaluate("2 + 3;"));
-        expect (6 == (int) engine.evaluate("2 * 3;"));
-        expect (4 == (int) engine.evaluate("Math.pow(2, 2);"));
+        expect (static_cast<int> (engine.evaluate("2 + 3;")) == 5);


The Yoda conditions...

jrlanglois · 2020-12-08T21:11:42Z

blueprint/blueprint.cpp

- * just not supported without the 8.1 dll available.
- */
-#if defined (_WIN32) || defined (_WIN64)
-#define DUK_USE_DATE_NOW_WINDOWS 1


Ah damn... I removed this accidentally, and also I don't have a Win7 system to test on anymore.

… the module into a professional looking public framework.

JoshMarler · 2020-12-09T15:41:51Z

@jrlanglois this looks great. A Herculean effort!

One key point, It's a pretty enormous diff! This is quite likely to cause merge conflicts upon rebasing etc as I suspect some other things will get merged first, any chance of breaking it up a little? i.e. the build/module changes as one PR. Stylistic changes as another.

Couple other points.

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot. The vast majority of documentation will focus on the JS level using the existing docsify system. Nick and I were discussing this a while back and we'd probably aim to keep the vast majority of documentation in one place. So we'd probably document those two classes in docsify pages and largely avoid doxygen.

In regards to the various formatting changes, an ideal thing here would be a clang-format config. I think there are some out in the wild on the JUCE forum which follow the JUCE style guide. Obviously this is separate from the overall refactors preferring auto etc which I think look great. Appreciate it'd be rather painful to separate those refactors from formatting though ....

@nick-thompson what do you think?

olidacombe · 2020-12-11T12:49:20Z

In regards to the various formatting changes, an ideal thing here would be a clang-format config. I think there are some out in the wild on the JUCE forum which follow the JUCE style guide. Obviously this is separate from the overall refactors preferring auto etc which I think look great. Appreciate it'd be rather painful to separate those refactors from formatting though ....

Yeah I think clang-format and prettier config in repo, along with some checks in CI for compliance and probably some easily-initialised pre-commit hooks will be super valuable. No reason to waste anybody's time worrying about formatting down the line when a lot of that burden can be tooled away. Seems you've already decided to do this anyway #24

jrlanglois · 2020-12-11T14:41:00Z

Thanks for the feedback folks! I'm glad to see there's interest. As I mentioned in my original post - treat this T H I C C PR as merely conceptual. Though looking back on it, it's fairly straightforward to break all of this down into sizeable chunks.

@JoshMarler

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot.

I'd like to make the case that JUCE users will be interested in this, what with it being a JUCE module... This means C++ will likely be the first thing people think of, particularly when it comes to rolling out their own C++ classes for the JS mechanisms to interact with. Documentation will be good to have in place and will make it more digestible.

This might be less useful in granular terms (eg: ShadowView), but for the classes that users are most likely to touch it seems worthwhile.

Also, keeping in mind the freshness and early stages of this module, docs might help debug when things go awry.

JoshMarler · 2020-12-11T14:44:23Z

Thanks for the feedback folks! I'm glad to see there's interest. As I mentioned in my original post - treat this T H I C C PR as merely conceptual. Though looking back on it, it's fairly straightforward to break all of this down into sizeable chunks.

@JoshMarler

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot.

I'd like to make the case that JUCE users will be interested in this, what with it being a JUCE module... This means C++ will likely be the first thing people think of, particularly when it comes to rolling out their own C++ classes for the JS mechanisms to interact with. Documentation will be good to have in place and will make it more digestible.

This might be less useful in granular terms (eg: ShadowView), but for the classes that users are most likely to touch it seems worthwhile.

Also, keeping in mind the freshness and early stages of this module, docs might help debug when things go awry.

Totally fair comment. It would be nice if there was a clean single solution to generate docs for both sides of the API if possible. @nick-thompson, any thoughts on this?

nick-thompson · 2020-12-12T21:47:25Z

Wow! @jrlanglois herculean effort indeed, very very much appreciated. And cheers to each of you for the discussion here.

High level comments: yea clang-format + prettier + automated clang-tidy/whatever CI checks is definitely the sustainable approach here, and one that we've planned to do but not gotten around to. Documentation, yea, @JoshMarler said it well; we're definitely aligning behind the idea of keeping the docs in one place, and that the most of the documentation will (and should) be geared towards the js side, so I'm starting to think it would be more maintenace than value to also support doxygen for the C++ side. That said, adding those docblock comments will indeed be helpful for contributors looking to understand the project, so I'm in favor of at least keeping up with our comments in the code!

@jrlanglois I see the breakout PRs, I'll go through and address each individually in more detail. Shall we consider this one closed then?

nick-thompson · 2020-12-12T21:54:17Z

Ah, misread my notifications, I'm seeing now that those were issues opened, not breakout PRs! Ignore the end of my previous comment then :)

I'll read through the issues and leave comments in detail where I have them though. And yes, to @JoshMarler's other point, I think breaking this down into bite sized PRs will be the way forward!

jrlanglois marked this pull request as draft December 8, 2020 20:35

jrlanglois force-pushed the Overhaul branch from 6f5367c to db58bcf Compare December 8, 2020 20:42

jrlanglois commented Dec 8, 2020

View reviewed changes

jrlanglois force-pushed the Overhaul branch from db58bcf to 567c281 Compare December 8, 2020 21:08

jrlanglois commented Dec 8, 2020

View reviewed changes

jrlanglois force-pushed the Overhaul branch from 567c281 to 11eaa8a Compare December 9, 2020 00:54

Overhauled the formatting and designs to improve readability and hone…

2706a90

… the module into a professional looking public framework.

jrlanglois force-pushed the Overhaul branch from 11eaa8a to 2706a90 Compare December 9, 2020 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhauled the formatting and designs. #152

Overhauled the formatting and designs. #152

jrlanglois commented Dec 8, 2020 •

edited

Loading

jrlanglois commented Dec 8, 2020

jrlanglois Dec 8, 2020

JoshMarler Dec 9, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020 •

edited

Loading

JoshMarler Dec 9, 2020

jrlanglois Dec 11, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020 •

edited

Loading

JoshMarler Dec 9, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020 •

edited

Loading

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020 •

edited

Loading

JoshMarler Dec 9, 2020

jrlanglois Dec 8, 2020

jrlanglois Dec 8, 2020

JoshMarler commented Dec 9, 2020 •

edited

Loading

olidacombe commented Dec 11, 2020

jrlanglois commented Dec 11, 2020

JoshMarler commented Dec 11, 2020

nick-thompson commented Dec 12, 2020

nick-thompson commented Dec 12, 2020

Overhauled the formatting and designs. #152

Are you sure you want to change the base?

Overhauled the formatting and designs. #152

Conversation

jrlanglois commented Dec 8, 2020 • edited Loading

There are many changes here

Still in progress

Why are you doing this?! ARE YOU MAD?!

jrlanglois commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrlanglois Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrlanglois Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrlanglois Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrlanglois Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshMarler commented Dec 9, 2020 • edited Loading

olidacombe commented Dec 11, 2020

jrlanglois commented Dec 11, 2020

JoshMarler commented Dec 11, 2020

nick-thompson commented Dec 12, 2020

nick-thompson commented Dec 12, 2020

jrlanglois commented Dec 8, 2020 •

edited

Loading

jrlanglois Dec 8, 2020 •

edited

Loading

jrlanglois Dec 8, 2020 •

edited

Loading

jrlanglois Dec 8, 2020 •

edited

Loading

jrlanglois Dec 8, 2020 •

edited

Loading

JoshMarler commented Dec 9, 2020 •

edited

Loading