[Big change] Migration to React Hooks

[da-h]

This PR migrates most React components of this project to the newer hook based react style.

Description

This PR applies three main changes to js files of the code base:

1. Migrating most React components (all but EmbeddingsPane and App) to hook based react functions.
2. Solving some minor bugs on the way:
   • added missing tbody tags in PropertyLists. (The browser console did mention this in its warnings).
   • added missing key-attributes in Pane. (The browser console did mention this in its warnings).
   • with the new react style the changing-properties-feature stopped working consistently in Pane and in PropertiesPane.
     Specifying to call stopPropagation only in PropertiesPane solved the issue. (However, I do not really understand why. Any idea? 😅 )
3. Code normalization.
   • Removing unneeded functions.
     I.e. getWindowSize and getContentSize have not been used in Pane & resize has not been used in both, Pane and PropertiesPane.
   • Rearranging all code blocks in the touched files so that all files have a similar structure in this order:
     • state variables
     • public events (overwritable by props)
     • private events (not overwritable by props)
     • (other, e.g. the calculations of Image position and scale in ImagePane)
     • rendering
   • Especially in the beginning of my work for this PR, I've found myself a bit lost in some quite long functions. Thus, I've added code comment headings for easier navigation in the larger files (see ImagePane).

Notes:

• The third point in the list is actually optional to accomplish the goal of this PR and rather a personal taste of mine to put more structure into some of the longer files. To make this step optional, all changes for this step are in separate commits (those named js: code normalize X).
  Feel free to remove or adapt in case something does not meet the code style of the project.
• I also appreciate any suggestions for improvement, especially considering that this change is rather huge and i'm quite new to react and react hooks.

Motivation and Context

This change has been suggested in a previous discussion, and I am also sure that future changes would benefit of a more up-to-date code base.

How Has This Been Tested?

No additional testing. As no functionality should have been changed the cypress tests should succeed.

Types of changes

• Refactor
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[JackUrb]

Hi @da-h, this is really exciting! Just getting back from a long vacation, but I'll jump into reviewing this as soon as I'm caught up with everything.

[JackUrb]

Alright, sorry this took so long to get back to @da-h. Overall though, this is an amazing change. Honestly I have an easier time parsing through and reading your new take on the code than I do trying to understand the old code, and I wrote most of the original stuff! (albeit years ago now)

Specifying to call stopPropagation only in PropertiesPane solved the issue. (However, I do not really understand why. Any idea?)

Are you saying that, if you add the blurStopPropogation chain to the underlying PropertyItem, but then only set it in PropertiesPane, that fixes both the issue in PropertiesPane and in the regular Pane? That's really weird, and would point me to a difference in event propagation behavior by default in the Pane. Might remain something of a mystery...

As no functionality should have been changed the cypress tests should succeed.

This is a huge accomplishment really, there was a lot of code here to keep right, so the fact that tests are still passing is great.

[JackUrb]

As a tiny nit, a common react pattern when we know that this component uses specific props and the downstream component (that we extend to with {...props}) uses different ones is to do parameter de-structuring inline, like:

function ImagePane({ known, used, values, ...props})

This both clarifies values that this component needs as well as prevents them from being passed down.

In this particular class though, there's a lot of stuff that is being used here and still passed down to the pane, so for these I like doing a de-structure right at the top:

const {appApi, height, width, other, args, that, are, needed} = props;

This still makes it easy to reason about the required prop arguments upfront.

[da-h]

Thanks for pointing this out, I did not know that syntax, yet! :)

I have adapted all Components to include such a de-construction of all prop-values (if possible) at the start of the function.

I have added that change as a new commit for easier inspection of the changes.
(If you want, I can squash these changes also into the respective previous commits before merging).

[da-h]

Hey there!,

this time it was me who was on the road. :)

Specifying to call stopPropagation only in PropertiesPane solved the issue. (However, I do not really understand why. Any idea?)

Are you saying that, if you add the blurStopPropogation chain to the underlying PropertyItem, but then only set it in PropertiesPane, that fixes both the issue in PropertiesPane and in the regular Pane? That's really weird, and would point me to a difference in event propagation behavior by default in the Pane. Might remain something of a mystery...

It seems so. I would have expected that both Types (PropertiesPane and Pane) would not change the behavior of the respective Child-Elements that actually set values (i.e. the PropertyItems). Maybe there is a special case somewhere in main.js checking the parent class that I have not yet seen. However, the current constellation now seems to work consistently, and the comment at that line of code suggests to future contributors that any change of this function possibly needs additional thoughts. :)

[da-h]

This is strange. The test on the current master branch (without the changes of this PR) has failed. Let's see if I find something.

[da-h]

Ah too bad, thought the "update to master" button would rebase instead of creating a merge commit.

Too keep things clean, I can remove the merge commit tomorrow an rebase the changes. (Unfortunately, i do not find a possibility to do that on the road).

[da-h]

Strange thing that the test fails again, especially because I have tested both this PR and the fix on my github fork and also locally without any failing test.
I will look into that and propose anotherr fix tomorrow.

[JackUrb]

Ah too bad, thought the "update to master" button would rebase instead of creating a merge commit.

Too keep things clean, I can remove the merge commit tomorrow an rebase the changes. (Unfortunately, i do not find a possibility to do that on the road).

No worries here, I've been able to keep track!

[da-h]

Hey, just found the errors & resubmitted the fixed PR.

short summary:

• i messed up the cypress initialization by forgetting to set an important flag (see the fix in Bugfix for job visual-regression-test-init (Reverts quickfix #863) #866). The tests here should pass now mostly, however, without the fix, they are not very conclusive, yet. With the fix in place they are however succeeding on my fork.
• there were some little differences to the old implementation that caused the rest of the errors. Changes in more detail:
  • barwidgets in the panes have been copied on every click, so the cypress error was indeed legitimate here
  • the line smoothing slider did not show the chosen value before the code refactoring due to a typo (i adapted to match the previous behavior for now)
  • the Property Lists have been missing tbody tags. adding those resulted in a slightly different formatting. (i adapted the css file to match the previous formatting as well). Also I fixed a mistake i did in rewriting the code with the de-construct syntax.

On the consistency of the tests:
I have also tested the consistency of the cypress tests on the way by resubmitting the exact same PR multiple times to my fork.
I found that two tests (misc_video_tensor and image_save_jpeg) did fail once in 50 tests or so. Possibly we should add a short sleep-command there to make this consistent across all testing machines where loading times may differ slightly.
On the other hand, I would like to refactor the main.js as well to reduce the number of state-change-triggered redraws as much as possible, which should also solve some of these problems.

(In the example image below, all branches test the exact same changes. Here, the image_save_jpeg has failed in test 4).

[da-h]

Rebased onto master.

[JackUrb]

Another great QoL change for the codebase here, thanks again @da-h!