Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Papyros integration #3401

Closed
wants to merge 91 commits into from
Closed

Conversation

winniederidder
Copy link
Contributor

@winniederidder winniederidder commented Feb 16, 2022

This pull request integrates Papyros into Dodona to allow users to write and run code within their browser.

Papyros makes use of WebWorkers, requiring SharedArrayBuffers to communicate. Enabling this feature in the browser requires specific HTTP headers to be set on resources used in the pages where this feature is enabled. Examples of such resources are /packs and /assets. For production/staging, these headers must be enabled on the server. In development, this can be taken care of with configuration in config/development.rb .
Some HTTP headers must also be added to responses concerning exercises and their description to allow the iframes to load everything correctly and securely.

  • Tests were added
  • Documentation update can be found at dodona-edu/dodona-edu.github.io#

Closes #3232 .
Depends on #3366

@winniederidder
Copy link
Contributor Author

winniederidder commented Feb 20, 2022

Current visuals of the integration:
papyros-integration-v1

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the papyros initialization to a separate function instead of integrating it in initExerciseShow because we won't always run this code.

Layout:

  • The button to trigger papyros should not be a floating action button. What about something like https://cdn.phpjabbers.com/files/frees/sliding-contact-form.jpg? (I wouldn't show the button when expanded)
  • I wouldn't use "Papyros" branding in the editor. The title should be something like "Run Python code in your browser" so it's clear what this is. Maybe add a BETA label?
  • The help text should be bigger, especially since it's still in beta. Tweak the text so it's more aimed at something younger children can understand. Link them to the contact form instead of just saying to contact us. The margin should be in line with the rest of the page. The top needs a bit more spacing.
  • Run and stop buttons feel a bit off here design-wise. I don't have a clear suggestion at this time.
  • The vertical alignment for "waiting for input" if off.
  • It's not really clear what the output area is when you load the page now that the title is gone. Maybe add a "your output will appear here" placeholder?
  • The output area is too prominent compared to the editor itself. What about reducing the height a bit + increase the height of the editor area by giving it the leftover vertical space instead of a fixed height?
  • The copy button at the bottom isn't clear. There is enough space so at least include a label

@rien can you check the CSP header changes?

app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
app/assets/javascripts/exercise.js Outdated Show resolved Hide resolved
app/assets/stylesheets/components/offcanvas.css.scss Outdated Show resolved Hide resolved
@@ -177,6 +194,14 @@ def media
end
end

# Serve the inputServiceWorker file required to handle input in Papyros
# Could potentially changed to a Dodona-specific service worker importing a Papyros-function
def isw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a better name

# Serve the inputServiceWorker file required to handle input in Papyros
# Could potentially changed to a Dodona-specific service worker importing a Papyros-function
def isw
send_file(Rails.root.join('node_modules', '@dodona', 'papyros', 'dist', 'inputServiceWorker.js'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very ugly. In theory, the node_modules directory shouldn't even be available at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. A possible solution might be as follows:
I could try to make Papyros search for the serviceworker at a given path (using configureInput). We could then move the isw-path to a different controller (pages is probably not the most appropriate, but something like that) and give it a suitable name (something related to service_worker). Finally, I could add an installation script that copies the default service worker to a given path (such as the public directory or from wherever the application wants to serve the service worker if it doesn't provide one themselves).

app/views/activities/show.html.erb Outdated Show resolved Hide resolved
@@ -14,11 +14,15 @@
# Allow webpack-dev-server
policy.connect_src :self, Rails.configuration.tutor_url.to_s,
'https://*.googleapis.com',
'http://localhost:3035', 'ws://localhost:3035'
'http://localhost:3035', 'ws://localhost:3035',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is localhost used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Localhost was already present before this PR. I believe it has to do with the webpack-dev-server, as mentioned in the comment above. Someone else might be able to shed more light on this specifically (@rien , @jorg-vr ?)

@winniederidder
Copy link
Contributor Author

I would move the papyros initialization to a separate function instead of integrating it in initExerciseShow because we won't always run this code.

That method won't always be called, because you mentioned e.g. using it in the Grading page? Would a separate method that can be given an editor argument (in case of an exercise page) work then? Being able to access the ace-editor adds useful functionality which I would like to preserve.

Layout:

  • Run and stop buttons feel a bit off here design-wise. I don't have a clear suggestion at this time.

I could move the buttons away from the status bar and put them elsewhere (similar to futurecoder or Codecademy). This issue also describes the addition of keybinds which can replace the buttons, but students might prefer also having physical buttons in a good location.

  • It's not really clear what the output area is when you load the page now that the title is gone. Maybe add a "your output will appear here" placeholder?

The title attribute is used (shown when the user hovers over the output field). We can also add a title ourselves above the respective div, or go back to the placeholder.

  • The copy button at the bottom isn't clear. There is enough space so at least include a label

Is a floating action button okay here, or should the button style itself also be changed?

@winniederidder
Copy link
Contributor Author

Some late screenshots:
Button to show Papyros
papyros-button
Current layout with new button, beta label, ...
papyros-shown

@winniederidder winniederidder marked this pull request as ready for review February 28, 2022 18:36
@winniederidder winniederidder requested review from chvp and a team as code owners February 28, 2022 18:36
@winniederidder winniederidder changed the base branch from develop to jsbundling-rails March 1, 2022 13:41
@winniederidder
Copy link
Contributor Author

Closed in favor of #3453

@chvp chvp deleted the papyros-integration branch April 14, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial Papyros integration
4 participants