Capture code refactor for new interface #672

waridrox · 2021-07-02T21:19:03Z

Part of #645

Hi @jywarren, until further progress at the safari issue blocker, can we use this as an alternative.

I've implemented the following with this PR:

Refactoring the new capture code at new-capture.js file under assets that is only utilised by the new interface
Addition of login prompt modal (need a new capture controller to implement fully), overexposure warning messages.
Support for responsive layouts

I know it has a lot of changes but the problem is that the new-capture code requires the new interface which in turn requires the styles to function properly. So all of them are linked to each other...😅

I was thinking of implementing cross browser support on the old interface as well, but I think it will require some refactor of the old capture.js as well. So until the tests are implemented, I am putting that on hold...The asset pre-compilation test seems to be failing due to the use of ES6 arrow syntax under WebRTC.

spectral-workbench/app/assets/javascripts/new-capture.js

Lines 40 to 60 in 56e417e

    
           getUserMedia: function(options) { 
        
             AdapterJS.webRTCReady(() => { 
        
               // The WebRTC API is ready. 
        
               const container = document.getElementById('webcam'), 
        
                     video = document.createElement('video'); 
        
               container.appendChild(video); 
        
               video.autoplay = true; 
        
               video.id = 'webcam-video' 
        
               const successCallback = stream => { 
        
                 $('#heightIndicator').show() 
        
                 $('#webcam-msg').hide() 
        
                 attachMediaStream(video, stream) 
        
                 if ($W.flipped == true) { 
        
                   $W.flipped = false; // <= turn it false because f_h() will toggle it. messy. 
        
                   $W.flip_horizontal(); 
        
                 } 
        
               }; 
        
               const errorCallback = () => console.warn(error);

I've tried to fix it but still it's failing...it's related to Uglifier not being able to recognise the ES6 syntax.

gitpod-io · 2021-07-02T21:19:08Z

waridrox · 2021-07-20T00:23:28Z

For reference, here's the new interface in action:

new.mov

jywarren · 2021-07-20T19:03:03Z

app/helpers/application_helper.rb

  def webrtc?
-    request.env['HTTP_USER_AGENT'] && (request.env['HTTP_USER_AGENT'].match('Opera') || request.env['HTTP_USER_AGENT'].match('Firefox') || request.env['HTTP_USER_AGENT'].match('Chrome'))
+    request.env['HTTP_USER_AGENT'] && (request.env['HTTP_USER_AGENT'].match('Opera') || request.env['HTTP_USER_AGENT'].match('Safari') || request.env['HTTP_USER_AGENT'].match('Firefox') || request.env['HTTP_USER_AGENT'].match('Chrome'))


jywarren · 2021-07-20T19:03:29Z

app/views/capture/_save.html.erb

@@ -17,7 +17,7 @@

 <form class="form" action="/spectrums" method="post" id="saveForm" target="_blank" onSubmit="$W.cancelSave();window.location='#page1'">

-  <input placeholder="Title (required)" name="spectrum[title]" id="spectrum_notes" type="text" /><br />
+  <input placeholder="Title (required)" name="spectrum[title]" type="text" /><br />


Oops, are you sure here? This is the legacy capture interface, would this break it?

A warning was popping up in the console that there were 2 id's associated with the same name - spectrum_notes:

spectral-workbench/app/views/capture/_save.html.erb

Line 20 in 8f89e0f

<input placeholder="Title (required)" name="spectrum[title]" id="spectrum_notes" type="text" /><br />

spectral-workbench/app/views/capture/_save.html.erb

Line 55 in 8f89e0f

<textarea placeholder="Describe your spectrum: light source? Sample preparation? What's your goal?" style="span8" rows="6" name="spectrum[notes]" id="spectrum_notes"></textarea>

I don't see the id being used anywhere, so maybe it is ok to proceed 🤞

hm, ok! As long as you can confirm it's not broken, sounds good! And indeed, a quick search shows it's not used in code anywhere -- https://github.com/publiclab/spectral-workbench/search?q=spectrum_notes

looks like it was simply duplicated! Good catch!

jywarren · 2021-07-20T19:04:52Z

config/environments/production.rb

-  # Compress JavaScripts and CSS.
-  config.assets.js_compressor = :uglifier
+  # Compress JavaScripts and CSS (Using ES6 syntax).
+  config.assets.js_compressor = Uglifier.new(harmony: true)


Aha - ok so here we are missing something as it created this error:

Run bundle exec rake assets:precompile rake aborted! NameError: uninitialized constant Uglifier /home/runner/work/spectral-workbench/spectral-workbench/config/environments/production.rb:38:in `block in <top (required)>'

https://github.com/publiclab/spectral-workbench/pull/672/checks?check_run_id=3109251932#step:5:7

Do you need to include Uglifier or something?

This is actually supposed to make the uglifier compatible with the ES6 syntax used in new-capture.js according to lautis/uglifier#127 but still the problem remains unsolved...😕

So this is a new error uninitialised constant Uglifier which should be resolved by https://stackoverflow.com/questions/30338860/heroku-push-error-nameerror-uninitialized-constant-uglifierversion-on-rake but still the problem is persisting when I tried this out.

Hm, they note:

The problem seemed to be an incompatibility between Sprockets 3.1 and the Uglifier compressor version I was using, 2.1. Upgrading Uglifier by putting this line in my Gemfile resolved the issue:

gem 'uglifier', '~> 2.7'

Have you tried that? Else possibly @noi5e may be able to help a bit (or at least have some ideas?) as he's worked a lot with Rails precompilation in his React project!

OK here is another possibility:

sstephenson/sprockets#740

Installing the gem "therubyracer" and rolling uglifier back to 2.6.1 in the Gemfile ( gem 'uglifier', '~> 2.6.1' ) fixes the problem.

...i think we removed therubyracer? Yes -

spectral-workbench/Gemfile

Lines 16 to 18 in 8f89e0f

# gem 'therubyracer', :platforms => :ruby

gem 'uglifier', '>= 1.0.3'

Try turning it back on and then pegging the uglifier gem to v2.6.1?

For comparison, in plots2 --

https://github.com/publiclab/plots2/blob/4e52e2ea2875593734b96bac4e8326e058a25d8e/config/environments/production.rb#L18

jywarren · 2021-07-20T19:05:02Z

package.json

@@ -8,6 +8,7 @@
  "dependencies": {
    "ace-builds": "jywarren/ace-builds#v1.2.4",
    "bootstrap-css": "jozefizso/bower-bootstrap-css#~2.3.2",
+    "bs-stepper": "^1.7.0",


jywarren · 2021-07-20T19:07:26Z

Very cool @waridrox ! Looking awesome. I left some notes in the comments and once we get past this asset precompilation issue this looks good to merge!

jywarren · 2021-07-20T19:12:24Z

app/assets/javascripts/new-capture.js

@@ -0,0 +1,724 @@
+//= require graph.js


Oh wait, hmm, so, is what's going on here that you've made a copy of the older version of capture.js (from https://github.com/publiclab/spectral-workbench.js/blob/main/examples/capture/capture.js) with some modifications in order to make it safari compatible?

But I'm not sure I'm understanding what the changes were -- here's a diff: https://gist.github.com/jywarren/534b0d8dfb4e58f7833ac92bdee8cd20/revisions

I'm just not sure what those new sections auto_detect_sample_row etc are replacing - what older code are they overriding? I'd like to avoid duplicating the majority of this file if possible. What, as narrowly as possible, are the required changes we want to make here?

And if there is no other simpler way, what is our plan moving forward to stop using this redundant file and unify the codebase again?

So the new capture code in the sw.js lib at https://github.com/publiclab/spectral-workbench.js/blob/main/examples/capture/capture.js is making use of https://github.com/publiclab/spectral-workbench.js/blob/main/examples/capture/capture.js as well as https://github.com/publiclab/spectral-workbench.js/blob/main/dist/capture.dist.js. I've refactored the code to fit under one file at new-capture.js

Ah, hmm. I think perhaps this should be done upstream in that library, as well as perhaps in a separate PR. Is it possible to leave this file out and include exactly as it was in the demo?

<script src="../capture/capture.js" type="text/javascript"></script> <script src="../../dist/capture.dist.js" type="text/javascript"></script>

I just think this is a major change and a duplication of code with the upstream library. Keep this file though! the refactor is not a bad idea it's just something we should do in the pure JS library, as well as in a distinct PR.

Sure! but

<script src="../capture/capture.js" type="text/javascript"></script>

is also calling different functions like getRow, getUsermedia, detect_sample_row which are in different files at https://github.com/publiclab/spectral-workbench.js/tree/main/examples/capture

OK, but are those functions represented in other files also included from the new-capture/index.html?

https://github.com/publiclab/spectral-workbench.js/blob/4f29a95119af0aef7a4a2e131ec4815df475e1a8/examples/new-capture/index.html#L27-L39

Other than capture.dist.js and capture.js all other files are included as assets so I think there should be no problems there...

jywarren · 2021-07-20T19:18:13Z

app/views/capture/index2.html.erb

            <span class="bs-stepper-label">Capture</span>
+            <script>
+            function capturePage() {


Oof these are rough, huh? So basically the stepper plugin wasn't compatible with the canvas/getusermedia webcam stuff, when run in safari? Is this really the only way forward? It's so much extra code and work!

Could we at least move these all into one file so as not to mix them with the HTML?

The stepper is just stopping the graph so I am rendering all the content first and then hiding it with the scripts instead of using the default stepper behaviour. Sure we could move this to a separate file for a cleaner code.

We can also mark them as a temporary step. If we resolve the Safari issue, how much work would it be to revert back, and can we consolidate the code as well as clearly mark it with comments that it's addressing #665 and that upon solving that it could be removed and simplified?

Sure, the only thing that would be required in that case would be to simply remove the temporary script and add the pre-defined script section on the page which would use the buttons to toggle the divs that would then be rendered as different pages.

It may even be helpful to make this particular set of changes all in a single commit. Then we can point at that commit which will include the complete diff of just this steps-fix, and nothing else.

jywarren · 2021-07-20T19:18:55Z

app/views/capture/index2.html.erb

+                <div>
+                <p><small>Using calibration:</small></p>
+                <select name="spectrum[calibration_id]" class="select-calibration select-calibration-capture span2">
+                  <%= render partial: 'capture/calibrations', locals: { calibrations: @calibrations, calibration: @calibration } %>


Wow very cool! Can you screenshot this as well, for documentation?

It's still not functioning as intended (separated out) as of now since I tested it using the old capture controller. Will need to refactor this to the new capture controller.

It should be OK to use the same controller actions and routes between the two interfaces -- the functionality is the same, right?

jywarren · 2021-07-20T20:02:57Z

Also just want to say, great work here, and keep it up! There's a lot going on and more to tweak before we're ready, but I appreciate the hard work!

waridrox · 2021-07-26T13:13:39Z

@jywarren, I tried reverting changing the Uglifier version and installing rubyracer....still the asset pre-compilation fails 😞. @noi5e what are your thoughts on this ?

The error is NameError: uninitialized constant Uglifier

jywarren · 2021-07-27T20:43:04Z

Oof, ok i'm sorry this is such a blocker. I'll try to work on it today or tomorrow. If @noi5e is able to see any way around this too, that's great as well!

noi5e · 2021-07-27T21:20:37Z

I should be honest and say that I don't have the expertise in this that might be desired. 😬 BUT, I am willing to get my hands dirty, maybe it will help to have an extra set of eyes on this.

I'm going to start by forking the repo and setting up locally. The comments are a lot to sift through here! Something that would help me: can you please give me a step-by-step to reproduce the error?

waridrox · 2021-07-27T21:30:57Z

Sure @noi5e thanks a lot 🙌 !! The error is produced once the app gets ready for production and the assets pre-compile. We are using the ES6 arrow syntax in the sw.js lib at

https://github.com/publiclab/spectral-workbench.js/blob/4f29a95119af0aef7a4a2e131ec4815df475e1a8/dist/capture.dist.js#L259-L270

but the Uglifier is throwing an error

lautis/uglifier#127 was supposed to fix the ES6 issue but this created another one - NameError: uninitialized constant Uglifier

and so far we have tried -

but still no help 😕

jywarren · 2021-07-28T13:28:40Z

Hmm, @waridrox I was actually able to run rake assets:precompile without an error here in Gitpod:

https://gist.github.com/jywarren/79073c832009089c58a885cfc5891f84

I wonder why this is different from the tests. Can you reproduce the error in Gitpod? I had to install bundler, yarn install, etc. first.

waridrox · 2021-07-28T13:51:12Z

Oh @jywarren so could this be due to the bundler gem. I see the production environment set to false in the gitpod config file:

spectral-workbench/.gitpod.yml

Lines 6 to 15 in 7e1fb80

    
           tasks: 
        
               - init: > 
        
                   cp config/database.yml.gitpod config/database.yml && 
        
                   cp config/config.yml.example config/config.yml && 
        
                   cp db/schema.rb.example db/schema.rb && 
        
                   bundle install --without production && 
        
                   yarn install && 
        
                   rails db:create && 
        
                   rails db:schema:load && 
        
                   rails db:migrate

Also the commands like bundle install, yarn install are not executed automatically...which should otherwise execute as per the config 🤔

jywarren · 2021-07-28T16:15:45Z

Ahhhhh ok yes I need to set RAILS_ENV=production, thank you

…

On Wed, Jul 28, 2021, 9:51 AM Mohammad Warid ***@***.***> wrote: Oh @jywarren <https://github.com/jywarren> so could this be due to the bundler gem. I see the production environment set to false in the gitpod config file: https://github.com/publiclab/spectral-workbench/blob/7e1fb804500723e6dfd8dff557070acdea2e8897/.gitpod.yml#L6-L15 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JYIHY6K26CGHNMFQDLT2ADOXANCNFSM47XMNCGQ> .

waridrox · 2021-07-28T16:59:25Z

Yeah now we see the error...

jywarren · 2021-07-28T22:29:09Z

OK! I think i may have resolved it by using require 'uglifier' as in https://github.com/lautis/uglifier#usage

and by upgrading to the latest (v4) uglifier. Let's see. It'll need a rebase after but we can just see how it does here.

waridrox · 2021-08-02T08:21:43Z

Yay ✨ the test for asset pre-compilation passed!!

…ort and rear camera on mobile devices on old capture

jywarren · 2021-08-03T15:12:20Z

app/assets/stylesheets/new-capture.css

@@ -0,0 +1,610 @@
+* {


Let's de-duplicate this with https://github.com/publiclab/spectral-workbench.js/blob/main/examples/new-capture/new-capture.css - and name it differently so it's clear that it isn't redundant - so this file in this PR should have only those things that are specific to the Rails app here, and everything else should go upstream in the JS library, and then https://github.com/publiclab/spectral-workbench.js/blob/main/examples/new-capture/new-capture.css would be referenced in addition to the local version here from index2.html. Make sense?

@waridrox i liked what you said about including both stylesheets in succession. Let's do that!

waridrox · 2021-08-04T20:30:13Z

Reduced some css styling by utilising the already existing CSS styles from sw.js lib.
The files responsible for the new capture interface have been changed from new-capture.js and new-capture.css to new-interface.js and new-interface.css respectively.

jywarren · 2021-08-04T22:34:51Z

app/assets/stylesheets/new-interface.css

@@ -0,0 +1,463 @@
+@import 'spectral-workbench/examples/new-capture/new-capture.css';


jywarren · 2021-08-04T22:41:23Z

OK! I think this is good to merge! Just in case, in the future can you comment that you think it's ready? But since we've reviewed this a lot i think we're OK. Merging! Great job!!

noi5e · 2021-08-06T00:06:39Z

Great job here @waridrox!

waridrox force-pushed the captureRefactor branch 3 times, most recently from 13caa10 to 4abe613 Compare July 3, 2021 16:25

waridrox changed the title ~~Capture code refactor and responsive layout for new interface~~ Capture code refactor for both old and new interface Jul 3, 2021

waridrox mentioned this pull request Jul 3, 2021

[Planning] - spectralworkbench.js / rails integration #645

Closed

11 tasks

waridrox force-pushed the captureRefactor branch 2 times, most recently from a87589d to 74370b3 Compare July 13, 2021 07:19

waridrox changed the title ~~Capture code refactor for both old and new interface~~ Capture code refactor for old interface Jul 13, 2021

waridrox force-pushed the captureRefactor branch 2 times, most recently from 279e9c3 to a52a5a4 Compare July 13, 2021 11:41

waridrox mentioned this pull request Jul 17, 2021

New capture integration keeps stopping live graph on safari browsers #665

Open

waridrox force-pushed the captureRefactor branch from ed240c7 to 0bb9130 Compare July 19, 2021 23:39

waridrox requested a review from jywarren July 20, 2021 00:23

waridrox changed the title ~~Capture code refactor for old interface~~ Capture code refactor for new interface Jul 20, 2021

jywarren reviewed Jul 20, 2021

View reviewed changes

waridrox requested a review from jywarren July 26, 2021 13:13

waridrox force-pushed the captureRefactor branch 2 times, most recently from ec07d02 to 46ffb17 Compare July 29, 2021 11:50

waridrox added 11 commits August 3, 2021 18:26

New capture at /v2

e15a30c

removing duplicate id (2 id's on same page)

a79e5e4

Checking for webrtc on safari + Adding support for cross browser supp…

5dc6f7b

…ort and rear camera on mobile devices on old capture

Configuring uglifier to use ES6 syntax and utilising new files

d1835da

Using bs-stepper as dependency for new interface

f909f46

New interface with refactored capture code at new-capture.js

2798606

fixed wrong css syntax

667202c

utlising new-capture.js as temp file for scripts

56abe8c

Making use of the new capture code from sw.js library

2dde13b

adding a production env flag to precompile assets

afff528

updating Gemfile to latest

089e252

waridrox force-pushed the captureRefactor branch from 513b825 to 089e252 Compare August 3, 2021 13:00

jywarren reviewed Aug 3, 2021

View reviewed changes

utilising CSS styles from sw.js lib

af70753

waridrox requested a review from jywarren August 4, 2021 20:34

jywarren reviewed Aug 4, 2021

View reviewed changes

jywarren approved these changes Aug 4, 2021

View reviewed changes

jywarren merged commit 0a84661 into publiclab:main Aug 4, 2021

	getUserMedia: function(options) {
	AdapterJS.webRTCReady(() => {
	// The WebRTC API is ready.
	const container = document.getElementById('webcam'),
	video = document.createElement('video');

	container.appendChild(video);
	video.autoplay = true;
	video.id = 'webcam-video'

	const successCallback = stream => {
	$('#heightIndicator').show()
	$('#webcam-msg').hide()
	attachMediaStream(video, stream)
	if ($W.flipped == true) {
	$W.flipped = false; // <= turn it false because f_h() will toggle it. messy.
	$W.flip_horizontal();
	}
	};

	const errorCallback = () => console.warn(error);

	# gem 'therubyracer', :platforms => :ruby

	gem 'uglifier', '>= 1.0.3'

		@@ -0,0 +1,463 @@
		@import 'spectral-workbench/examples/new-capture/new-capture.css';

Capture code refactor for new interface #672

Capture code refactor for new interface #672

Conversation

waridrox commented Jul 2, 2021 • edited Loading

gitpod-io bot commented Jul 2, 2021

waridrox commented Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waridrox Jul 20, 2021 • 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

waridrox Jul 20, 2021 • 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

jywarren commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waridrox Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waridrox Jul 20, 2021 • 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

Choose a reason for hiding this comment

waridrox Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waridrox Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jul 20, 2021

waridrox commented Jul 26, 2021

jywarren commented Jul 27, 2021

noi5e commented Jul 27, 2021

waridrox commented Jul 27, 2021

jywarren commented Jul 28, 2021 • edited Loading

waridrox commented Jul 28, 2021 • edited Loading

jywarren commented Jul 28, 2021 via email

waridrox commented Jul 28, 2021 • edited Loading

jywarren commented Jul 28, 2021

waridrox commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waridrox commented Aug 4, 2021

Choose a reason for hiding this comment

jywarren commented Aug 4, 2021

noi5e commented Aug 6, 2021

waridrox commented Jul 2, 2021 •

edited

Loading

waridrox commented Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

waridrox Jul 20, 2021 •

edited

Loading

jywarren commented Jul 28, 2021 •

edited

Loading

waridrox commented Jul 28, 2021 •

edited

Loading

waridrox commented Jul 28, 2021 •

edited

Loading