Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add Paste code to submit feature #2720
base: main
Are you sure you want to change the base?
add Paste code to submit feature #2720
Changes from 1 commit
62985ba
56f076e
aa8ed35
3893628
3376c11
bf9d7bc
add44de
74e36d9
dbe2363
723f8d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this an
array_val
, then one can select which ones are enabled. By default let's enable upload for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but in that case, the cookies will also need a major revamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should prevent the array from being empty here. How should I handle it when someone submits the form with an empty array?
Should I use an alert?
I haven’t seen any other similar menus to refer to for this implementation.
Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check that the array doesn't get empty and if it does refuse to save or use a
danger
flash with the message. Alternative is to have a message in theconfig-check
page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think implementing the required restriction in the backend can be done in the next PR, as it's more general. Including it in this commit would make things confusing.
can i do it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the default or allowed submission methods? Also, it would be good to clarify that these are submission methods from the web interface, as there's also the submit client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowed submission methods, with the default set to upload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is submit client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This: https://raw.githubusercontent.com/DOMjudge/domjudge/refs/heads/main/submit/submit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use a simpler name here (and then of course use that as base for the entry point calculation). A nice simple naming scheme could be the
shortName
of thecontestProblem
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still want the pasted submission to have a special identifier in the filename, to differentiate it from uploads. Is it possible to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason the way of submitting is important? You could test with adding
fileupload-
to the name? You would need to test yourself if this still works.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it "fileupload"? I thought it would be something like "Paste."
However, I now think using "shortName" directly is fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think the short name as given by the contest problem is ok, let's use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the short name allows various characters. I will still use
preg_replace('/[^a-zA-Z0-9_.-]/', '_', $tempFileName);
to handle file naming using the short name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename as entry point does not work for Kotlin, nor for Java.
See https://github.com/DOMjudge/domjudge/blob/main/webapp/templates/team/partials/submit_scripts.html.twig#L3 or https://github.com/DOMjudge/domjudge/blob/main/submit/submit#L206
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered ignoring the entry in Paste mode, since only one file can be uploaded with Paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One example is Java, where the entry point basically specifies the class. If you would make the name of the file clear in the UI (or even an input that can be changed after selecting the problem) we could do the same auto-detection (with possibility to change it) with the entry point as for the uploaded file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's feasible in terms of implementation, but Paste code is essentially designed for convenience with a single file. If we allow changes to the file name, it somewhat defeats the original purpose of this feature. Alternatively, we could try handling languages where the entry point is not the file name with special treatment? (Actually, this seems similar to the original approach for uploads.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I'm currently teaching a data structures class where only C language is allowed, and the usage of Paste is significantly higher than that of upload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/as6325400/domjudge/blob/paste_code/webapp/templates/team/partials/submit_scripts.html.twig#L58-L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I followed that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I'd like to ask how I can reference code in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything else to adjust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-a-permanent-link-to-a-code-snippet#linking-to-code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of moving the language selection above the editor? This way you can also use it here to fix syntax highlighting in the editor (as idea).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea. When I first implemented it, I wasn't sure how to dynamically adjust the syntax highlighting in the editor by changing the language in the menu. Is that achievable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that depends on the twig template.
Let's leave that for a follow up PR in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's call the button just
Submit
, same with the other buttonThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both "Upload" and "Paste" are available at the same time, would it be weird or cause confusion if both buttons are labeled "Submit"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is confusing as in both cases you are submitting what you filled in in the form you are currently seeing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, I misunderstood what you meant.