Simplify submission source view logic

Kevinjil · Kevinjil · commit 3bfe7ae812ca · 2025-11-12T00:42:43.000+01:00
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.
diff --git a/webapp/public/js/domjudge.js b/webapp/public/js/domjudge.js
@@ -1361,7 +1361,12 @@ function initDiffEditor(editorId, deletedFiles) {
         }
     };
     wrapper.find(".nav").on('show.bs.tab', (e) => {
-        updateTabRank(e.target.dataset.rank);
+        if (e.target.dataset.rank.length > 0) {
+            const rank = parseInt(e.target.dataset.rank);
+            updateTabRank(rank);
+        } else {
+            updateTabRank(undefined);
+        }
     })
 
     const editor = {
@@ -1412,9 +1417,25 @@ function initDiffEditor(editorId, deletedFiles) {
     editor.onDiffSelectChange(updateSelect);
 }
 
-function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
+function ensureModel(models, submitId) {
+    if (submitId in models) {
+        const model = models[submitId];
+        if (!('model' in model)) {
+            model['model'] = monaco.editor.createModel(
+                model['source'],
+                undefined,
+                monaco.Uri.file("diff/" + submitId + "/" + model['filename'])
+            );
+        }
+        return model['model'];
+    }
     const empty = monaco.editor.getModel(monaco.Uri.file("empty")) ?? monaco.editor.createModel("", undefined, monaco.Uri.file("empty"));
+    return empty;
+}
+
+function initDiffEditorTab(editorId, diffId, submissionId, models) {
     const navItem = document.getElementById(`${diffId}-link`);
+    const isDeleted = !(submissionId in models);
 
     const diffEditor = monaco.editor.createDiffEditor(
         document.getElementById(diffId), {
@@ -1465,24 +1486,15 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
 
     const updateSelect = (submitId, noDiff) => {
         const exists = submitId in models;
-        if (rank === undefined) {
-            document.getElementById(diffId).parentElement.style.display = exists ? 'block' : 'none';
-            navItem.style.display = exists ? 'block' : 'none';
+        if (isDeleted) {
+            document.getElementById(diffId).parentElement.style.display = exists ? '' : 'none';
+            navItem.style.display = exists ? '' : 'none';
             if (!exists) return;
         }
 
-        const model = models[submitId] ??= {'model': empty};
-        if (!noDiff) {
-            if (!model['model']) {
-                model['model'] = monaco.editor.createModel(model['source'], undefined, monaco.Uri.file("test/" + submitId + "/" + model['filename']));
-            }
-        }
-
-        if (noDiff || !model['renamedFrom']) {
-            renamedFrom(undefined);
-        } else {
-            renamedFrom(model['renamedFrom']);
-        }
+        const model = models[submitId];
+        const notRenamed = noDiff || !model || !model['renamedFrom'];
+        renamedFrom(notRenamed ? undefined : model['renamedFrom']);
 
         diffEditor.updateOptions({
             renderOverviewRuler: !noDiff,
@@ -1496,10 +1508,11 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
             updateMode(editors[editorId].getDiffMode())
         }
         const oldViewState = diffEditor.saveViewState();
-        diffEditor.setModel({
-            original: noDiff ? modifiedModel : models[submitId]['model'],
-            modified: modifiedModel,
-        });
+        const x = {
+            original: noDiff ? ensureModel(models, submissionId) : ensureModel(models, submitId),
+            modified: ensureModel(models, submissionId),
+        };
+        diffEditor.setModel(x);
         diffEditor.restoreViewState(oldViewState);
 
         diffEditor.getOriginalEditor().updateOptions({
@@ -1514,25 +1527,30 @@ function initDiffEditorTab(editorId, diffId, rank, models, modifiedModel) {
     editors[editorId].onDiffSelectChange(updateSelect);
     updateSelect(editors[editorId].getDiffSelection(), editors[editorId].getDiffSelection() === "");
 
+    const setIcon = (icon) => {
+        const element = navItem.querySelector('.fa-fw');
+        element.className = 'fas fa-fw fa-' + icon;
+    };
     const updateIcon = () => {
-        if (rank === undefined) return;
-        const update = (icon) => {
-            const element = navItem.querySelector('.fa-fw');
-            element.className = 'fas fa-fw fa-' + icon;
-        };
+        if (isDeleted) {
+            setIcon('file-circle-minus');
+            return;
+        }
+
+        const submitId = parseInt(editors[editorId].getDiffSelection());
         const noDiff = editors[editorId].getDiffSelection() === "";
         if (noDiff) {
-            update('file');
+            setIcon('file');
             return;
         }
 
         const lineChanges = diffEditor.getLineChanges();
-        if (diffEditor.getModel().original == empty) {
-            update('file-circle-plus');
+        if (!(submitId in models)) {
+            setIcon('file-circle-plus');
         } else if (lineChanges !== null && lineChanges.length > 0) {
-            update('file-circle-exclamation');
+            setIcon('file-circle-exclamation');
         } else {
-            update('file-circle-check');
+            setIcon('file-circle-check');
         }
     }
     diffEditor.onDidUpdateDiff(updateIcon);
diff --git a/webapp/src/Controller/Jury/SubmissionController.php b/webapp/src/Controller/Jury/SubmissionController.php
@@ -839,17 +839,6 @@ public function sourceAction(
             return $response;
         }
 
-        /** @var SubmissionFile[] $files */
-        $files = $this->em->createQueryBuilder()
-            ->from(SubmissionFile::class, 'file')
-            ->select('file')
-            ->andWhere('file.submission = :submission')
-            ->setParameter('submission', $submission)
-            ->orderBy('file.ranknumber')
-            ->getQuery()
-            ->getResult();
-        // TODO: change array to `filename -> file` for efficiency of renaming?
-
         $otherSubmissions = [];
         $originalSubmission = $submission->getOriginalSubmission();
         if ($originalSubmission) {
@@ -893,69 +882,63 @@ public function sourceAction(
             $otherSubmissions[] = $oldSubmission;
         }
 
-        /** @var SubmissionFile[] $files */
+        $files_query = array_map(fn($s) => $s->getSubmitid(), $otherSubmissions);
+        $files_query[] = $submission->getSubmitid();
+        /** @var SubmissionFile[] $oldFiles */
         $oldFiles = $this->em->createQueryBuilder()
             ->from(SubmissionFile::class, 'file')
             ->select('file')
             ->andWhere('file.submission in (:submissions)')
-            ->setParameter('submissions', array_map(fn($s) => $s->getSubmitid(), $otherSubmissions))
+            ->setParameter('submissions', $files_query)
             ->orderBy('file.submission, file.ranknumber')
             ->getQuery()
             ->getResult();
 
-        $otherFiles = [];
+        /** @var array<string, array<int, array{
+         *      rank: int,
+         *      filename: string,
+         *      source: string,
+         *      renamedFrom?: string
+         * }>> $files */
+        $files = [];
+        /** @var (string|false)[] $renames */
+        $renames = [];
         foreach ($oldFiles as $f) {
             $submitId = $f->getSubmission()->getSubmitid();
-            $otherFiles[$submitId] ??= [];
-            $otherFiles[$submitId][$f->getFilename()] = [
+            $files[$f->getFilename()] ??= [];
+            $files[$f->getFilename()][$submitId] = [
+                'rank' => $f->getRank(),
                 'filename' => $f->getFilename(),
                 'source'   => mb_check_encoding($f->getSourcecode(), 'UTF-8') ? $f->getSourcecode() : "Could not display binary file",
             ];
-        }
 
-        // Handle file renaming for a single-file submission.
-        if (count($files) === 1) {
-            $f = $files[0];
-            foreach ($otherSubmissions as $s) {
-                $sf = $otherFiles[$s->getSubmitid()];
-                if (count($sf) === 1 && !array_key_exists($f->getFilename(), $sf)) {
-                    $oldName = array_key_first($sf);
-                    $otherFiles[$s->getSubmitid()] = [
-                        $f->getFilename() => [
-                            'renamedFrom' => $oldName,
-                            ...$sf[$oldName]
-                        ],
-                    ];
-                }
-            }
+            // Keep track of the single filename within a submission for handling renaming.
+            $renames[$submitId] = array_key_exists($submitId, $renames) ? false : $f->getFilename();
         }
 
-        $deletedFiles = [];
-        foreach ($otherSubmissions as $s) {
-            $submitId = $s->getSubmitid();
-            $sf = $otherFiles[$submitId];
-            if (count($files) === 1 && count($sf) === 1) {
-                continue;
-            }
-            foreach ($sf as $name => $file) {
-                if (!array_key_exists($name, $files)) {
-                    // TODO: note to self: rotated the key order s.t. we can iterate over deleted filenames in a.o. twig
-                    $deletedFiles[$name] ??= [];
-                    $deletedFiles[$name][$submitId] = $otherFiles[$submitId][$name];
+        // Handle file renaming for a single-file submission.
+        $renamedTo = $renames[$submission->getSubmitid()];
+        if ($renamedTo !== false) {
+            foreach ($renames as $submitId => $filename) {
+                if ($filename !== false && $filename !== $renamedTo) {
+                    $files[$renamedTo][$submitId] = $files[$filename][$submitId];
+                    $files[$renamedTo][$submitId]['renamedFrom'] = $filename;
+                    unset($files[$filename][$submitId]);
+                    if (count($files[$filename]) === 0) {
+                        unset($files[$filename]);
+                    }
                 }
             }
         }
-        dump($deletedFiles);
 
+        ksort($files);
         return $this->render('jury/submission_source.html.twig', [
             'submission' => $submission,
             'files' => $files,
             'oldSubmission' => $oldSubmission,
             'originalSubmission' => $originalSubmission,
             'allowEdit' => $this->allowEdit(),
             'otherSubmissions' => $otherSubmissions,
-            'otherFiles' => $otherFiles,
-            'deletedFiles' => $deletedFiles,
         ]);
     }
 
diff --git a/webapp/src/Twig/TwigExtension.php b/webapp/src/Twig/TwigExtension.php
@@ -73,7 +73,6 @@ public function getFunctions(): array
             new TwigFunction('globalBannerAssetPath', $this->dj->globalBannerAssetPath(...)),
             new TwigFunction('shadowMode', $this->shadowMode(...)),
             new TwigFunction('showDiff', $this->showDiff(...), ['is_safe' => ['html']]),
-            new TwigFunction('showDeleted', $this->showDeleted(...), ['is_safe' => ['html']]),
         ];
     }
 
@@ -958,56 +957,24 @@ public function getMonacoModel(SubmissionFile $file): string
         );
     }
 
-    /** @param array<int, SubmissionFile[]> $otherFiles */
-    public function showDiff(string $editorId, string $diffId, SubmissionFile $newFile, array $otherFiles): string
+    /** @param array<int, array{
+     *      rank: int,
+     *      filename: string,
+     *      source: string,
+     *      renamedFrom?: string
+     * }> $files */
+    public function showDiff(string $editorId, string $diffId, int $submissionId, string $filename, array $files): string
     {
         $editor = <<<HTML
 <div class="editor" id="$diffId"></div>
 <script>
 $(function() {
     const editorId = '%s';
     const diffId = '%s';
-    const rank = %d;
+    const submissionId = %d;
     const models = %s;
     require(['vs/editor/editor.main'], () => {
-        const modifiedModel = %s;
-        initDiffEditorTab(editorId, diffId, rank, models, modifiedModel);
-    });
-});
-</script>
-HTML;
-
-        $others = [];
-        foreach ($otherFiles as $submissionId => $files) {
-            if (isset($files[$newFile->getFilename()])) {
-                // TODO: add `tag` containing `previous` / `original`
-                $others[$submissionId] = $files[$newFile->getFilename()];
-            }
-        }
-
-        return sprintf(
-            $editor,
-            $editorId,
-            $diffId,
-            $newFile->getRank(),
-            $this->serializer->serialize($others, 'json'),
-            $this->getMonacoModel($newFile),
-        );
-    }
-
-    /** @param array<int, SubmissionFile[]> $deletedFiles */
-    public function showDeleted(string $editorId, string $diffId, string $filename, array $deletedFiles): string
-    {
-        $editor = <<<HTML
-<div class="editor" id="$diffId"></div>
-<script>
-$(function() {
-    const editorId = '%s';
-    const diffId = '%s';
-    const models = %s;
-    require(['vs/editor/editor.main'], () => {
-        const modifiedModel = monaco.editor.getModel(monaco.Uri.file("empty"));
-        initDiffEditorTab(editorId, diffId, undefined, models, modifiedModel);
+        initDiffEditorTab(editorId, diffId, submissionId, models);
     });
 });
 </script>
@@ -1017,7 +984,8 @@ public function showDeleted(string $editorId, string $diffId, string $filename,
             $editor,
             $editorId,
             $diffId,
-            $this->serializer->serialize($deletedFiles, 'json'),
+            $submissionId,
+            $this->serializer->serialize($files, 'json'),
         );
     }
 
diff --git a/webapp/templates/jury/partials/submission_diff.html.twig b/webapp/templates/jury/partials/submission_diff.html.twig
@@ -2,19 +2,18 @@
 {# Mark the first tab that is shown as active. #}
 {% set extra_css_classes = "active" %}
 <ul class="nav nav-tabs source-tab-nav align-items-end">
-    {%- for file in files %}
-        {% set diff_id = "diff" ~ file.submitfileid %}
+    {%- for name, name_files in files %}
+        {% set diff_id = "diff-" ~ name %}
+        {% if name_files[submission.submitid] is defined %}        
+            {% set rank = name_files[submission.submitid].rank %}
+        {% else %}
+            {% set rank = null %}
+        {% endif %}
         <li class="nav-item">
-            <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>
+            <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>
         </li>
         {% set extra_css_classes = "" %}
     {%- endfor %}
-    {%- for deletedName, _ in deletedFiles %}
-        {% set diff_id = "diff-deleted-" ~ deletedName %}
-        <li class="nav-item">
-            <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>
-        </li>
-    {%- endfor -%}
 
             <li class="nav-item flex-grow-1 text-end mb-1">
                 <a class="download btn btn-secondary btn-sm"
@@ -56,27 +55,18 @@
 <script>
 $(() => {
     require(['vs/editor/editor.main'], () => {
-        const deletedFiles = {{ deletedFiles | json_encode | raw }};
-        initDiffEditor('{{ editor_id }}', deletedFiles);
+        initDiffEditor('{{ editor_id }}');
     });
 });
 </script>
 {% set extra_css_classes = "show active" %}
 <div class="tab-content source-tab">
-    {%- for file in files %}
-    {# TODO: rewrite s.t. files is also a nice array like otherFiles #}
-    {# TODO: allow null to showDiff work as deleted file #}
-        {% set diff_id = "diff" ~ file.submitfileid %}
+    {%- for name, name_files in files %}
+        {% set diff_id = "diff-" ~ name %}
         <div class="tab-pane fade {{ extra_css_classes }}" id="{{ diff_id }}-tab" role="tabpanel">
-            {{ showDiff(editor_id, diff_id, file, otherFiles) }}
+            {{ showDiff(editor_id, diff_id, submission.submitid, name, name_files) }}
         </div>
         {% set extra_css_classes = "" %}
     {%- endfor %}
-    {%- for deletedName, deletedF in deletedFiles %}
-        {% set diff_id = "diff-deleted-" ~ deletedName %}
-        <div class="tab-pane fade source-deleted" id="{{ diff_id }}-tab" role="tabpanel">
-            {{ showDeleted(editor_id, diff_id, deletedName, deletedF) }}
-        </div>
-    {%- endfor %}
 </div>
 </div>
diff --git a/webapp/templates/jury/submission_source.html.twig b/webapp/templates/jury/submission_source.html.twig
