Skip to content

Commit 3bfe7ae

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 7605184 commit 3bfe7ae

File tree

5 files changed

+103
-177
lines changed

5 files changed

+103
-177
lines changed

‎webapp/public/js/domjudge.js‎

Lines changed: 49 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1361,7 +1361,12 @@ function initDiffEditor(editorId, deletedFiles) {
13611361
}
13621362
};
13631363
wrapper.find(".nav").on('show.bs.tab', (e) => {
1364-
updateTabRank(e.target.dataset.rank);
1364+
if (e.target.dataset.rank.length > 0) {
1365+
const rank = parseInt(e.target.dataset.rank);
1366+
updateTabRank(rank);
1367+
} else {
1368+
updateTabRank(undefined);
1369+
}
13651370
})
13661371

13671372
const editor = {
@@ -1412,9 +1417,25 @@ function initDiffEditor(editorId, deletedFiles) {
14121417
editor.onDiffSelectChange(updateSelect);
14131418
}
14141419

1415-
function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
1420+
function ensureModel(models, submitId) {
1421+
if (submitId in models) {
1422+
const model = models[submitId];
1423+
if (!('model' in model)) {
1424+
model['model'] = monaco.editor.createModel(
1425+
model['source'],
1426+
undefined,
1427+
monaco.Uri.file("diff/" + submitId + "/" + model['filename'])
1428+
);
1429+
}
1430+
return model['model'];
1431+
}
14161432
const empty = monaco.editor.getModel(monaco.Uri.file("empty")) ?? monaco.editor.createModel("", undefined, monaco.Uri.file("empty"));
1433+
return empty;
1434+
}
1435+
1436+
function initDiffEditorTab(editorId, diffId, submissionId, models) {
14171437
const navItem = document.getElementById(`${diffId}-link`);
1438+
const isDeleted = !(submissionId in models);
14181439

14191440
const diffEditor = monaco.editor.createDiffEditor(
14201441
document.getElementById(diffId), {
@@ -1465,24 +1486,15 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
14651486

14661487
const updateSelect = (submitId, noDiff) => {
14671488
const exists = submitId in models;
1468-
if (rank === undefined) {
1469-
document.getElementById(diffId).parentElement.style.display = exists ? 'block' : 'none';
1470-
navItem.style.display = exists ? 'block' : 'none';
1489+
if (isDeleted) {
1490+
document.getElementById(diffId).parentElement.style.display = exists ? '' : 'none';
1491+
navItem.style.display = exists ? '' : 'none';
14711492
if (!exists) return;
14721493
}
14731494

1474-
const model = models[submitId] ??= {'model': empty};
1475-
if (!noDiff) {
1476-
if (!model['model']) {
1477-
model['model'] = monaco.editor.createModel(model['source'], undefined, monaco.Uri.file("test/" + submitId + "/" + model['filename']));
1478-
}
1479-
}
1480-
1481-
if (noDiff || !model['renamedFrom']) {
1482-
renamedFrom(undefined);
1483-
} else {
1484-
renamedFrom(model['renamedFrom']);
1485-
}
1495+
const model = models[submitId];
1496+
const notRenamed = noDiff || !model || !model['renamedFrom'];
1497+
renamedFrom(notRenamed ? undefined : model['renamedFrom']);
14861498

14871499
diffEditor.updateOptions({
14881500
renderOverviewRuler: !noDiff,
@@ -1496,10 +1508,11 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
14961508
updateMode(editors[editorId].getDiffMode())
14971509
}
14981510
const oldViewState = diffEditor.saveViewState();
1499-
diffEditor.setModel({
1500-
original: noDiff ? modifiedModel : models[submitId]['model'],
1501-
modified: modifiedModel,
1502-
});
1511+
const x = {
1512+
original: noDiff ? ensureModel(models, submissionId) : ensureModel(models, submitId),
1513+
modified: ensureModel(models, submissionId),
1514+
};
1515+
diffEditor.setModel(x);
15031516
diffEditor.restoreViewState(oldViewState);
15041517

15051518
diffEditor.getOriginalEditor().updateOptions({
@@ -1514,25 +1527,30 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
15141527
editors[editorId].onDiffSelectChange(updateSelect);
15151528
updateSelect(editors[editorId].getDiffSelection(), editors[editorId].getDiffSelection() === "");
15161529

1530+
const setIcon = (icon) => {
1531+
const element = navItem.querySelector('.fa-fw');
1532+
element.className = 'fas fa-fw fa-' + icon;
1533+
};
15171534
const updateIcon = () => {
1518-
if (rank === undefined) return;
1519-
const update = (icon) => {
1520-
const element = navItem.querySelector('.fa-fw');
1521-
element.className = 'fas fa-fw fa-' + icon;
1522-
};
1535+
if (isDeleted) {
1536+
setIcon('file-circle-minus');
1537+
return;
1538+
}
1539+
1540+
const submitId = parseInt(editors[editorId].getDiffSelection());
15231541
const noDiff = editors[editorId].getDiffSelection() === "";
15241542
if (noDiff) {
1525-
update('file');
1543+
setIcon('file');
15261544
return;
15271545
}
15281546

15291547
const lineChanges = diffEditor.getLineChanges();
1530-
if (diffEditor.getModel().original == empty) {
1531-
update('file-circle-plus');
1548+
if (!(submitId in models)) {
1549+
setIcon('file-circle-plus');
15321550
} else if (lineChanges !== null && lineChanges.length > 0) {
1533-
update('file-circle-exclamation');
1551+
setIcon('file-circle-exclamation');
15341552
} else {
1535-
update('file-circle-check');
1553+
setIcon('file-circle-check');
15361554
}
15371555
}
15381556
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 (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 binary file",
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)