JSON Integration in Runner

[SantamRC]

I added the JSON integration functionality for the runner. The way I am approaching it is that after the file is dragged and dropped in the main window, the file is checked whether it is valid JSON or not. Then the data is stored in sessionStorage . The study window fetches this data from it's own renderer process and reads the title of the study and inserts it into the study window. I can build the rest of the study UI component rendering based on this approach. But I would really love to know what you think about it and if you can suggest any improvements to this.

[FelixHenninger]

Hi @SantamRC , thanks once more for this contribution, I appreciate it! A couple of comments real quick:

• I really love the integration of electron-reloader, which I think is an excellent idea that's going to make things a lot easier! 👍 I think we can pull that in the repo straight away.
• For the JSON logic, I'm afraid that things aren't quite as simple:
  • The PR code disables the StudyWindow logic, and shows the loaded files instead in the MainWindow. This is a neat proof-of-concept for sure, and a good first step, but to adopt the code into the main branch I'd like to either (a) see it building more directly on the existing code or (b) have a discussion about why the change is necessary. I'm open to changing the way the app works, but I would like to see some arguments about why a large-scale change is required now. From my perspective, the isolation of the study and the runner UI is something I'd like to keep, and is also (I think) necessary to implement the UI you propose in your GSoC application. If this is temporary, that's of course completely ok, but in that case I would wait with merging the code at the current stage.
  • To make the JSON transfer possible in the current logic, the main window is configured with enableRemoteModule: true. I'm hesitant to add this change, because in my experience using the remote module is an anti-pattern that carries severe security and performance penalties; it's also deprecated by electron, with the explicit note that "there is almost always a better way to accomplish your task than using this module". As before, I'm open for a discussion around this, but I would like to see some arguments before making the change.

Reading your code, I remember well how annoying it was to fight with the IPC channels and get things to work in the convoluted electron APIs. I would love there to be a better way, and I would be thrilled to hear if if you know one. Even though the current code is complex and understandably annoying, my understanding is that it's following current best practices.

So in sum, I'm glad you're giving this a go, and thankful for your efforts -- let's have a conversation around the architectural changes and the overall design! Best, -Felix

[SantamRC]

Hey @FelixHenninger nice to hear it. Let's have a discussion about it and decide the architectural design of the application.
I have added few details to my proposal, can you take a final look at it before I submit the final propoasl :)

Best,
Santam.

[SantamRC]

@FelixHenninger do check the latest commit I made to the PR. The Session Storage Approach does not involve the remote module. I apologize for it, I might have overlooked that line of the code while cleaning it up for the PR.

[FelixHenninger]

No worries @SantamRC , good to hear that the proof of concept works without the remote module! 👍 What I'm not so sure about is whether you can transfer data between windows using this mechanism -- please bear in mind that right now the transfer takes place within the same window, as part of the same browser session. I'm not very confident that this will work as an application-wide data transfer mechanism -- but I'd be happy to be proven wrong (again) 😅

[SantamRC]

@FelixHenninger Do check out the latest commit I made. In electron since sessionStorage and localStorage works the same way as in browsers, a new window will not be able to access the sessionStorage object of another Window therefore for this purpose I have used localStorage and it works perfectly fine and can be considered a good proof of concept of my approach. Do tell me what you think about it :)

[SantamRC]

@FelixHenninger I have made a proof of concept of the IPC Approach as well. When I included redux in the proposal I planned an idea that whenever the states needs to be changed and transfered to other React Component we can use it instead of changing the data in the local storage everytime. But yes here I have created an IPC approach proof of concept where we can used the Redux State management as well. The IPC approach here will include transfering the JSON data from main renderer to the study window and then the study window react component's redux states can transfer that data to the other components and use accordingly.
I have put forth both the approaches proof of concept, do have a look at them and tell what you think of them and finally which one do you think is more feasible and is good to go with?
Btw, I really had fun studying the approaches and creating proof of concepts . I hope these proof of concepts help the community in the future if any such similar debates arise :)

[SantamRC]

hey @FelixHenninger I have added the IPC Approach to the proposal. Please take a look at it before I wrap it up as the final version before submitting it.