Skip to content

Commit 5f2da6b

Browse files
KevinjilKevinjil
committed
Simplify submission source view logic
As we are showing a diff editor with the option of not showing a diff at all, there is no longer a need to make a distinction between having a diff to show and not having a diff to show in the submission_source twig template. This enables us to unify the files array, s.t. it contains all relevant source code indexed by filename and submission respectively. This simplifies the logic needed for adding tabs for deleted files significantly, and removes some duplicated code.
1 parent 83bbfb4 commit 5f2da6b

File tree

5 files changed

+104
-178
lines changed

5 files changed

+104
-178
lines changed

‎webapp/public/js/domjudge.js‎

Lines changed: 50 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1309,7 +1309,7 @@ const disableButton = (btn) => {
13091309
}
13101310

13111311
const editors = [];
1312-
function initDiffEditor(editorId, deletedFiles) {
1312+
function initDiffEditor(editorId) {
13131313
const wrapper = $(`#${editorId}-wrapper`);
13141314

13151315
const initialTag = getDiffTag();
@@ -1354,7 +1354,12 @@ function initDiffEditor(editorId, deletedFiles) {
13541354
}
13551355
};
13561356
wrapper.find(".nav").on('show.bs.tab', (e) => {
1357-
updateTabRank(e.target.dataset.rank);
1357+
if (e.target.dataset.rank.length > 0) {
1358+
const rank = parseInt(e.target.dataset.rank);
1359+
updateTabRank(rank);
1360+
} else {
1361+
updateTabRank(undefined);
1362+
}
13581363
})
13591364

13601365
const editor = {
@@ -1405,9 +1410,25 @@ function initDiffEditor(editorId, deletedFiles) {
14051410
editor.onDiffSelectChange(updateSelect);
14061411
}
14071412

1408-
function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
1413+
function ensureModel(models, submitId) {
1414+
if (submitId in models) {
1415+
const model = models[submitId];
1416+
if (!('model' in model)) {
1417+
model['model'] = monaco.editor.createModel(
1418+
model['source'],
1419+
undefined,
1420+
monaco.Uri.file("diff/" + submitId + "/" + model['filename'])
1421+
);
1422+
}
1423+
return model['model'];
1424+
}
14091425
const empty = monaco.editor.getModel(monaco.Uri.file("empty")) ?? monaco.editor.createModel("", undefined, monaco.Uri.file("empty"));
1426+
return empty;
1427+
}
1428+
1429+
function initDiffEditorTab(editorId, diffId, submissionId, models) {
14101430
const navItem = document.getElementById(`${diffId}-link`);
1431+
const isDeleted = !(submissionId in models);
14111432

14121433
const diffEditor = monaco.editor.createDiffEditor(
14131434
document.getElementById(diffId), {
@@ -1458,24 +1479,15 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
14581479

14591480
const updateSelect = (submitId, noDiff) => {
14601481
const exists = submitId in models;
1461-
if (rank === undefined) {
1462-
document.getElementById(diffId).parentElement.style.display = exists ? 'block' : 'none';
1463-
navItem.style.display = exists ? 'block' : 'none';
1482+
if (isDeleted) {
1483+
document.getElementById(diffId).parentElement.style.display = exists ? '' : 'none';
1484+
navItem.style.display = exists ? '' : 'none';
14641485
if (!exists) return;
14651486
}
14661487

1467-
const model = models[submitId] ??= {'model': empty};
1468-
if (!noDiff) {
1469-
if (!model['model']) {
1470-
model['model'] = monaco.editor.createModel(model['source'], undefined, monaco.Uri.file("test/" + submitId + "/" + model['filename']));
1471-
}
1472-
}
1473-
1474-
if (noDiff || !model['renamedFrom']) {
1475-
renamedFrom(undefined);
1476-
} else {
1477-
renamedFrom(model['renamedFrom']);
1478-
}
1488+
const model = models[submitId];
1489+
const notRenamed = noDiff || !model || !model['renamedFrom'];
1490+
renamedFrom(notRenamed ? undefined : model['renamedFrom']);
14791491

14801492
diffEditor.updateOptions({
14811493
renderOverviewRuler: !noDiff,
@@ -1489,10 +1501,11 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
14891501
updateMode(editors[editorId].getDiffMode())
14901502
}
14911503
const oldViewState = diffEditor.saveViewState();
1492-
diffEditor.setModel({
1493-
original: noDiff ? modifiedModel : models[submitId]['model'],
1494-
modified: modifiedModel,
1495-
});
1504+
const x = {
1505+
original: noDiff ? ensureModel(models, submissionId) : ensureModel(models, submitId),
1506+
modified: ensureModel(models, submissionId),
1507+
};
1508+
diffEditor.setModel(x);
14961509
diffEditor.restoreViewState(oldViewState);
14971510

14981511
diffEditor.getOriginalEditor().updateOptions({
@@ -1507,25 +1520,30 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
15071520
editors[editorId].onDiffSelectChange(updateSelect);
15081521
updateSelect(editors[editorId].getDiffSelection(), editors[editorId].getDiffSelection() === "");
15091522

1523+
const setIcon = (icon) => {
1524+
const element = navItem.querySelector('.fa-fw');
1525+
element.className = 'fas fa-fw fa-' + icon;
1526+
};
15101527
const updateIcon = () => {
1511-
if (rank === undefined) return;
1512-
const update = (icon) => {
1513-
const element = navItem.querySelector('.fa-fw');
1514-
element.className = 'fas fa-fw fa-' + icon;
1515-
};
1528+
if (isDeleted) {
1529+
setIcon('file-circle-minus');
1530+
return;
1531+
}
1532+
1533+
const submitId = parseInt(editors[editorId].getDiffSelection());
15161534
const noDiff = editors[editorId].getDiffSelection() === "";
15171535
if (noDiff) {
1518-
update('file');
1536+
setIcon('file');
15191537
return;
15201538
}
15211539

15221540
const lineChanges = diffEditor.getLineChanges();
1523-
if (diffEditor.getModel().original == empty) {
1524-
update('file-circle-plus');
1541+
if (!(submitId in models)) {
1542+
setIcon('file-circle-plus');
15251543
} else if (lineChanges !== null && lineChanges.length > 0) {
1526-
update('file-circle-exclamation');
1544+
setIcon('file-circle-exclamation');
15271545
} else {
1528-
update('file-circle-check');
1546+
setIcon('file-circle-check');
15291547
}
15301548
}
15311549
diffEditor.onDidUpdateDiff(updateIcon);

‎webapp/src/Controller/Jury/SubmissionController.php‎

Lines changed: 30 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -839,17 +839,6 @@ public function sourceAction(
839839
return $response;
840840
}
841841

842-
/** @var SubmissionFile[] $files */
843-
$files = $this->em->createQueryBuilder()
844-
->from(SubmissionFile::class, 'file')
845-
->select('file')
846-
->andWhere('file.submission = :submission')
847-
->setParameter('submission', $submission)
848-
->orderBy('file.ranknumber')
849-
->getQuery()
850-
->getResult();
851-
// TODO: change array to `filename -> file` for efficiency of renaming?
852-
853842
$otherSubmissions = [];
854843
$originalSubmission = $submission->getOriginalSubmission();
855844
if ($originalSubmission) {
@@ -893,69 +882,63 @@ public function sourceAction(
893882
$otherSubmissions[] = $oldSubmission;
894883
}
895884

896-
/** @var SubmissionFile[] $files */
885+
$files_query = array_map(fn($s) => $s->getSubmitid(), $otherSubmissions);
886+
$files_query[] = $submission->getSubmitid();
887+
/** @var SubmissionFile[] $oldFiles */
897888
$oldFiles = $this->em->createQueryBuilder()
898889
->from(SubmissionFile::class, 'file')
899890
->select('file')
900891
->andWhere('file.submission in (:submissions)')
901-
->setParameter('submissions', array_map(fn($s) => $s->getSubmitid(), $otherSubmissions))
892+
->setParameter('submissions', $files_query)
902893
->orderBy('file.submission, file.ranknumber')
903894
->getQuery()
904895
->getResult();
905896

906-
$otherFiles = [];
897+
/** @var array<string, array<int, array{
898+
* rank: int,
899+
* filename: string,
900+
* source: string,
901+
* renamedFrom?: string
902+
* }>> $files */
903+
$files = [];
904+
/** @var array<int, (string|false)> $renames */
905+
$renames = [];
907906
foreach ($oldFiles as $f) {
908907
$submitId = $f->getSubmission()->getSubmitid();
909-
$otherFiles[$submitId] ??= [];
910-
$otherFiles[$submitId][$f->getFilename()] = [
908+
$files[$f->getFilename()] ??= [];
909+
$files[$f->getFilename()][$submitId] = [
910+
'rank' => $f->getRank(),
911911
'filename' => $f->getFilename(),
912912
'source' => mb_check_encoding($f->getSourcecode(), 'UTF-8') ? $f->getSourcecode() : "Could not display file as UTF-8, is it binary?",
913913
];
914-
}
915914

916-
// Handle file renaming for a single-file submission.
917-
if (count($files) === 1) {
918-
$f = $files[0];
919-
foreach ($otherSubmissions as $s) {
920-
$sf = $otherFiles[$s->getSubmitid()];
921-
if (count($sf) === 1 && !array_key_exists($f->getFilename(), $sf)) {
922-
$oldName = array_key_first($sf);
923-
$otherFiles[$s->getSubmitid()] = [
924-
$f->getFilename() => [
925-
'renamedFrom' => $oldName,
926-
...$sf[$oldName]
927-
],
928-
];
929-
}
930-
}
915+
// Keep track of the single filename within a submission for handling renaming.
916+
$renames[$submitId] = array_key_exists($submitId, $renames) ? false : $f->getFilename();
931917
}
932918

933-
$deletedFiles = [];
934-
foreach ($otherSubmissions as $s) {
935-
$submitId = $s->getSubmitid();
936-
$sf = $otherFiles[$submitId];
937-
if (count($files) === 1 && count($sf) === 1) {
938-
continue;
939-
}
940-
foreach ($sf as $name => $file) {
941-
if (!array_key_exists($name, $files)) {
942-
// TODO: note to self: rotated the key order s.t. we can iterate over deleted filenames in a.o. twig
943-
$deletedFiles[$name] ??= [];
944-
$deletedFiles[$name][$submitId] = $otherFiles[$submitId][$name];
919+
// Handle file renaming for a single-file submission.
920+
$renamedTo = $renames[$submission->getSubmitid()];
921+
if ($renamedTo !== false) {
922+
foreach ($renames as $submitId => $filename) {
923+
if ($filename !== false && $filename !== $renamedTo) {
924+
$files[$renamedTo][$submitId] = $files[$filename][$submitId];
925+
$files[$renamedTo][$submitId]['renamedFrom'] = $filename;
926+
unset($files[$filename][$submitId]);
927+
if (count($files[$filename]) === 0) {
928+
unset($files[$filename]);
929+
}
945930
}
946931
}
947932
}
948-
dump($deletedFiles);
949933

934+
ksort($files);
950935
return $this->render('jury/submission_source.html.twig', [
951936
'submission' => $submission,
952937
'files' => $files,
953938
'oldSubmission' => $oldSubmission,
954939
'originalSubmission' => $originalSubmission,
955940
'allowEdit' => $this->allowEdit(),
956941
'otherSubmissions' => $otherSubmissions,
957-
'otherFiles' => $otherFiles,
958-
'deletedFiles' => $deletedFiles,
959942
]);
960943
}
961944

‎webapp/src/Twig/TwigExtension.php‎

Lines changed: 11 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@ public function getFunctions(): array
7373
new TwigFunction('globalBannerAssetPath', $this->dj->globalBannerAssetPath(...)),
7474
new TwigFunction('shadowMode', $this->shadowMode(...)),
7575
new TwigFunction('showDiff', $this->showDiff(...), ['is_safe' => ['html']]),
76-
new TwigFunction('showDeleted', $this->showDeleted(...), ['is_safe' => ['html']]),
7776
];
7877
}
7978

@@ -958,56 +957,24 @@ public function getMonacoModel(SubmissionFile $file): string
958957
);
959958
}
960959

961-
/** @param array<int, SubmissionFile[]> $otherFiles */
962-
public function showDiff(string $editorId, string $diffId, SubmissionFile $newFile, array $otherFiles): string
960+
/** @param array<int, array{
961+
* rank: int,
962+
* filename: string,
963+
* source: string,
964+
* renamedFrom?: string
965+
* }> $files */
966+
public function showDiff(string $editorId, string $diffId, int $submissionId, string $filename, array $files): string
963967
{
964968
$editor = <<<HTML
965969
<div class="editor" id="$diffId"></div>
966970
<script>
967971
$(function() {
968972
const editorId = '%s';
969973
const diffId = '%s';
970-
const rank = %d;
974+
const submissionId = %d;
971975
const models = %s;
972976
require(['vs/editor/editor.main'], () => {
973-
const modifiedModel = %s;
974-
initDiffEditorTab(editorId, diffId, rank, models, modifiedModel);
975-
});
976-
});
977-
</script>
978-
HTML;
979-
980-
$others = [];
981-
foreach ($otherFiles as $submissionId => $files) {
982-
if (isset($files[$newFile->getFilename()])) {
983-
// TODO: add `tag` containing `previous` / `original`
984-
$others[$submissionId] = $files[$newFile->getFilename()];
985-
}
986-
}
987-
988-
return sprintf(
989-
$editor,
990-
$editorId,
991-
$diffId,
992-
$newFile->getRank(),
993-
$this->serializer->serialize($others, 'json'),
994-
$this->getMonacoModel($newFile),
995-
);
996-
}
997-
998-
/** @param array<int, SubmissionFile[]> $deletedFiles */
999-
public function showDeleted(string $editorId, string $diffId, string $filename, array $deletedFiles): string
1000-
{
1001-
$editor = <<<HTML
1002-
<div class="editor" id="$diffId"></div>
1003-
<script>
1004-
$(function() {
1005-
const editorId = '%s';
1006-
const diffId = '%s';
1007-
const models = %s;
1008-
require(['vs/editor/editor.main'], () => {
1009-
const modifiedModel = monaco.editor.getModel(monaco.Uri.file("empty"));
1010-
initDiffEditorTab(editorId, diffId, undefined, models, modifiedModel);
977+
initDiffEditorTab(editorId, diffId, submissionId, models);
1011978
});
1012979
});
1013980
</script>
@@ -1017,7 +984,8 @@ public function showDeleted(string $editorId, string $diffId, string $filename,
1017984
$editor,
1018985
$editorId,
1019986
$diffId,
1020-
$this->serializer->serialize($deletedFiles, 'json'),
987+
$submissionId,
988+
$this->serializer->serialize($files, 'json'),
1021989
);
1022990
}
1023991

‎webapp/templates/jury/partials/submission_diff.html.twig‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,18 @@
22
{# Mark the first tab that is shown as active. #}
33
{% set extra_css_classes = "active" %}
44
<ul class="nav nav-tabs source-tab-nav align-items-end">
5-
{%- for file in files %}
6-
{% set diff_id = "diff" ~ file.submitfileid %}
5+
{%- for name, name_files in files %}
6+
{% set diff_id = "diff-" ~ name %}
7+
{% if name_files[submission.submitid] is defined %}
8+
{% set rank = name_files[submission.submitid].rank %}
9+
{% else %}
10+
{% set rank = null %}
11+
{% endif %}
712
<li class="nav-item">
8-
<a id="{{ diff_id }}-link" class="nav-link {{ extra_css_classes }}" data-bs-toggle="tab" data-rank="{{ file.rank }}" href="#{{ diff_id }}-tab" role="tab"><i class="fas fa-fw fa-file"></i> {{ file.filename }}</a>
13+
<a id="{{ diff_id }}-link" class="nav-link {{ extra_css_classes }}" data-bs-toggle="tab" data-rank="{{ rank }}" href="#{{ diff_id }}-tab" role="tab"><i class="fas fa-fw fa-file"></i> {{ name }}</a>
914
</li>
1015
{% set extra_css_classes = "" %}
1116
{%- endfor %}
12-
{%- for deletedName, _ in deletedFiles %}
13-
{% set diff_id = "diff-deleted-" ~ deletedName %}
14-
<li class="nav-item">
15-
<a id="{{ diff_id }}-link" class="nav-link source-deleted" data-bs-toggle="tab" href="#{{ diff_id }}-tab" role="tab"><i class="fas fa-fw fa-file-circle-minus"></i> {{ deletedName }}</a>
16-
</li>
17-
{%- endfor -%}
1817

1918
<li class="nav-item flex-grow-1 text-end mb-1">
2019
<a class="download btn btn-secondary btn-sm"
@@ -56,27 +55,18 @@
5655
<script>
5756
$(() => {
5857
require(['vs/editor/editor.main'], () => {
59-
const deletedFiles = {{ deletedFiles | json_encode | raw }};
60-
initDiffEditor('{{ editor_id }}', deletedFiles);
58+
initDiffEditor('{{ editor_id }}');
6159
});
6260
});
6361
</script>
6462
{% set extra_css_classes = "show active" %}
6563
<div class="tab-content source-tab">
66-
{%- for file in files %}
67-
{# TODO: rewrite s.t. files is also a nice array like otherFiles #}
68-
{# TODO: allow null to showDiff work as deleted file #}
69-
{% set diff_id = "diff" ~ file.submitfileid %}
64+
{%- for name, name_files in files %}
65+
{% set diff_id = "diff-" ~ name %}
7066
<div class="tab-pane fade {{ extra_css_classes }}" id="{{ diff_id }}-tab" role="tabpanel">
71-
{{ showDiff(editor_id, diff_id, file, otherFiles) }}
67+
{{ showDiff(editor_id, diff_id, submission.submitid, name, name_files) }}
7268
</div>
7369
{% set extra_css_classes = "" %}
7470
{%- endfor %}
75-
{%- for deletedName, deletedF in deletedFiles %}
76-
{% set diff_id = "diff-deleted-" ~ deletedName %}
77-
<div class="tab-pane fade source-deleted" id="{{ diff_id }}-tab" role="tabpanel">
78-
{{ showDeleted(editor_id, diff_id, deletedName, deletedF) }}
79-
</div>
80-
{%- endfor %}
8171
</div>
8272
</div>

0 commit comments

Comments
 (0)