improve global feedback display #159

src/main/java/edu/kit/kastel/sdq/artemis4j/grading/Assessment.java

@@ -3,11 +3,15 @@
 
 import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import edu.kit.kastel.sdq.artemis4j.ArtemisNetworkException;
@@ -16,7 +20,9 @@
 import edu.kit.kastel.sdq.artemis4j.client.FeedbackType;
 import edu.kit.kastel.sdq.artemis4j.client.ProgrammingSubmissionDTO;
 import edu.kit.kastel.sdq.artemis4j.client.ResultDTO;
+import edu.kit.kastel.sdq.artemis4j.grading.location.ComparatorUtils;
 import edu.kit.kastel.sdq.artemis4j.grading.location.Location;
+import edu.kit.kastel.sdq.artemis4j.grading.location.LocationFormatter;
 import edu.kit.kastel.sdq.artemis4j.grading.metajson.AnnotationMappingException;
 import edu.kit.kastel.sdq.artemis4j.grading.metajson.MetaFeedbackMapper;
 import edu.kit.kastel.sdq.artemis4j.grading.penalty.GradingConfig;
@@ -48,13 +54,29 @@ public class Assessment extends ArtemisConnectionHolder {
     private static final FormatString GLOBAL_FEEDBACK_MISTAKE_TYPE_HEADER_NONSCORING =
             new FormatString(new MessageFormat("    * {0}:"));
     private static final FormatString GLOBAL_FEEDBACK_ANNOTATION =
-            new FormatString(new MessageFormat("        * {0} " + "at line {1,number}"));
+            new FormatString(new MessageFormat("        * {0} at line {1}"));
+    private static final FormatString GLOBAL_FEEDBACK_ANNOTATION_MULTIPLE =
+            new FormatString(new MessageFormat("        * {0} at lines {1}"));
     private static final FormatString GLOBAL_FEEDBACK_ANNOTATION_CUSTOM_PENALTY =
-            new FormatString(new MessageFormat("        * {0} at line {1,number} ({2,number,##.###}P)"));
+            new FormatString(new MessageFormat("        * {0} at line {1} ({2,number,##.###}P)"));
+    private static final FormatString GLOBAL_FEEDBACK_ANNOTATION_CUSTOM_PENALTY_MULTIPLE =
+            new FormatString(new MessageFormat("        * {0} at lines {1} ({2,number,##.###}P)"));
     private static final FormatString GLOBAL_FEEDBACK_LIMIT_OVERRUN = new FormatString(
             new MessageFormat("    * Note:" + " The sum of penalties hit the limits for this rating group."));
     private static final FormatString NO_FEEDBACK_DUMMY = new FormatString("The tutor has made no annotations.");
 
+    /**
+     * The global feedback containing a list of all annotations that were made, might be too large. In this case,
+     * the feedback is split into multiple parts. Artemis will display feedbacks that subtract points in a different
+     * section than feedbacks that do not subtract points.
+     * <p>
+     * With only the first feedback deducting points, the rest would be displayed in the info section. To prevent this,
+     * the other parts are passed a negative score that is as close to zero as possible.
+     * <p>
+     * This negative score is defined by the following constant.
+     */
+    private static final double GLOBAL_FEEDBACK_OTHER_PARTS_SCORE = -Double.MIN_VALUE;
+
     private final ResultDTO lockingResult;
     private final List<Annotation> annotations;
     private final List<TestResult> testResults;
@@ -123,7 +145,11 @@ public List<Annotation> getAnnotations() {
      * @return An unmodifiable list of annotations, possibly empty but never null.
      */
     public List<Annotation> getAnnotations(MistakeType mistakeType) {
-        return this.annotations.stream()
+        return this.getAnnotations(mistakeType, this.annotations);
+    }
+
+    private List<Annotation> getAnnotations(MistakeType mistakeType, Collection<Annotation> annotations) {
+        return annotations.stream()
                 .filter(a -> a.getMistakeType().equals(mistakeType))
                 .toList();
     }
@@ -397,13 +423,8 @@ private void internalSaveOrSubmit(boolean shouldSubmit) throws AnnotationMapping
                 this.programmingSubmission.getId(), relativeScore, feedbacks, this.lockingResult);
 
         // Sanity check
-        double feedbackPoints = Math.min(
-                Math.max(
-                        result.feedbacks().stream()
-                                .mapToDouble(FeedbackDTO::credits)
-                                .sum(),
-                        0.0),
-                this.getMaxPoints());
+        double feedbackPoints = Math.clamp(
+                result.feedbacks().stream().mapToDouble(FeedbackDTO::credits).sum(), 0.0, this.getMaxPoints());
         if (Math.abs(absoluteScore - feedbackPoints) > 1e-7) {
             throw new IllegalStateException("Feedback points do not match the calculated points. Calculated "
                     + absoluteScore + " but feedbacks sum up to " + feedbackPoints + " points.");
@@ -442,7 +463,7 @@ private List<FeedbackDTO> packAssessmentForArtemis() throws AnnotationMappingExc
                 .map(this::createInlineFeedback)
                 .toList());
 
-        // We have on (or more if they are too long) global feedback per rating group
+        // We have one (or more if they are too long) global feedback per rating group
         // These feedbacks deduct points
         feedbacks.addAll(this.config.getRatingGroups().stream()
                 .flatMap(r -> this.createGlobalFeedback(r).stream())
@@ -472,8 +493,8 @@ private List<FeedbackDTO> packAssessmentForArtemis() throws AnnotationMappingExc
         return feedbacks;
     }
 
-    private FeedbackDTO createInlineFeedback(Map.Entry<Integer, List<Annotation>> annotations) {
-        var sampleAnnotation = annotations.getValue().get(0);
+    private FeedbackDTO createInlineFeedback(Map.Entry<Integer, ? extends List<Annotation>> annotations) {
+        var sampleAnnotation = annotations.getValue().getFirst();
 
         String text =
                 "File " + sampleAnnotation.getFilePathWithoutType() + " at line " + sampleAnnotation.getDisplayLine();
@@ -511,6 +532,40 @@ private FeedbackDTO createInlineFeedback(Map.Entry<Integer, List<Annotation>> an
         return FeedbackDTO.newManual(0.0, text, reference, detailText);
     }
 
+    private TranslatableString formatGlobalFeedbackAnnotations(List<Annotation> annotations, boolean hasScore) {
+        LocationFormatter formatter = new LocationFormatter()
+                // .enableLineMerging()
+                .removeSharedPrefix(true)
+                // only show the starting line number
+                .setLocationToString(location -> "" + (location.start().line() + 1));
+
+        for (var annotation : annotations) {
+            formatter.addLocation(annotation.getLocation());
+        }
+
+        String filePath = annotations.getFirst().getFilePath();
+
+        if (hasScore) {
+            double customScore = annotations.stream()
+                    .mapToDouble(a -> a.getCustomScore().get())
+                    .sum();
+
+            FormatString formatString = GLOBAL_FEEDBACK_ANNOTATION_CUSTOM_PENALTY_MULTIPLE;
+            if (annotations.size() == 1) {
+                formatString = GLOBAL_FEEDBACK_ANNOTATION_CUSTOM_PENALTY;
+            }
+
+            return formatString.format(filePath, formatter.format(), customScore);
+        } else {
+            FormatString formatString = GLOBAL_FEEDBACK_ANNOTATION_MULTIPLE;
+            if (annotations.size() == 1) {
+                formatString = GLOBAL_FEEDBACK_ANNOTATION;
+            }
+
+            return formatString.format(filePath, formatter.format());
+        }
+    }
+
     /**
      * This builds one (or more if the feedback is too long) global feedback for a
      * rating group. The feedback deducts points, and lists all annotations that are
@@ -525,35 +580,61 @@ private List<FeedbackDTO> createGlobalFeedback(RatingGroup ratingGroup) {
         var header = GLOBAL_FEEDBACK_HEADER.format(
                 ratingGroup.getDisplayName(), points.score(), ratingGroup.getMinPenalty(), ratingGroup.getMaxPenalty());
 
-        // First collect only the lines so that we can later split the feedback by lines
-        List<TranslatableString> lines = new ArrayList<>();
+        // group annotations by mistake type
+        List<Map.Entry<MistakeType, List<Annotation>>> annotationsByType = new ArrayList<>();
         for (var mistakeType : ratingGroup.getMistakeTypes()) {
+            var annotationsWithType = this.getAnnotations(mistakeType, this.annotations);
+            if (!annotationsWithType.isEmpty()) {
+                annotationsByType.add(Map.entry(mistakeType, annotationsWithType));
+            }
+        }
+
+        // Sort the entries by their size, then by their elements location
+        annotationsByType.sort(Map.Entry.comparingByValue(ComparatorUtils.shortestFirst(
+                ComparatorUtils.compareByElement(Comparator.comparing(Annotation::getLocation)))));
+
+        // First collect only the lines so that we can later split the feedback by lines
+        Collection<TranslatableString> lines = new ArrayList<>();
+        for (var entry : annotationsByType) {
+            MistakeType mistakeType = entry.getKey();
+            List<Annotation> annotationsWithType = entry.getValue();
+
             Optional<Points> mistakePoints = this.calculatePointsForMistakeType(mistakeType);
-            if (mistakePoints.isPresent()) {
-
-                // Header per mistake type
-                if (ratingGroup.isScoringGroup() && mistakeType.shouldScore()) {
-                    lines.add(GLOBAL_FEEDBACK_MISTAKE_TYPE_HEADER.format(
-                            mistakeType.getButtonText(), mistakePoints.get().score()));
-                } else {
-                    // We don't want to display points if the rating group does not score
-                    lines.add(GLOBAL_FEEDBACK_MISTAKE_TYPE_HEADER_NONSCORING.format(mistakeType.getButtonText()));
-                }
+            if (mistakePoints.isEmpty()) {
+                continue;
+            }
 
+            // Header per mistake type
+            if (ratingGroup.isScoringGroup() && mistakeType.shouldScore()) {
+                lines.add(GLOBAL_FEEDBACK_MISTAKE_TYPE_HEADER.format(
+                        mistakeType.getButtonText(), mistakePoints.get().score()));
+            } else {
+                // We don't want to display points if the rating group does not score
+                lines.add(GLOBAL_FEEDBACK_MISTAKE_TYPE_HEADER_NONSCORING.format(mistakeType.getButtonText()));
+            }
+
+            var annotationsByFilePath = annotationsWithType.stream()
+                    .collect(Collectors.groupingBy(Annotation::getFilePath, LinkedHashMap::new, Collectors.toList()));
+
+            for (var annotations : annotationsByFilePath.values()) {
                 // Individual annotations
-                for (var annotation : this.getAnnotations(mistakeType)) {
-                    // For custom annotations, we have '* <file> at line <line> (<score>P)'
-                    // Otherwise, it's just '* <file> at line <line>'
-                    // Lines are zero-indexed
-                    if (annotation.getCustomScore().isPresent() && ratingGroup.isScoringGroup()) {
-                        lines.add(GLOBAL_FEEDBACK_ANNOTATION_CUSTOM_PENALTY.format(
-                                annotation.getFilePath(),
-                                annotation.getDisplayLine(),
-                                annotation.getCustomScore().get()));
-                    } else {
-                        lines.add(GLOBAL_FEEDBACK_ANNOTATION.format(
-                                annotation.getFilePath(), annotation.getDisplayLine()));
-                    }
+                Predicate<Annotation> hasScore = a -> a.getCustomScore().isPresent() && ratingGroup.isScoringGroup();
+
+                // separate annotations with and without score
+                List<Annotation> annotationsWithScore =
+                        annotations.stream().filter(hasScore).toList();
+                List<Annotation> annotationsWithoutScore =
+                        annotations.stream().filter(Predicate.not(hasScore)).toList();
+
+                // For custom annotations, we have '* <file> at line <line> (<score>P)'
+                if (!annotationsWithScore.isEmpty()) {
+                    lines.add(this.formatGlobalFeedbackAnnotations(annotationsWithScore, true));
+                }
+
+                // Otherwise, it's just '* <file> at line <line>'
+                // Lines are zero-indexed
+                if (!annotationsWithoutScore.isEmpty()) {
+                    lines.add(this.formatGlobalFeedbackAnnotations(annotationsWithoutScore, false));
                 }
             }
         }
@@ -580,8 +661,11 @@ private List<FeedbackDTO> createGlobalFeedback(RatingGroup ratingGroup) {
             List<FeedbackDTO> feedbacks = new ArrayList<>();
             for (int i = 0; i < feedbackTexts.size(); i++) {
                 // Only the first feedback deducts points
-                feedbacks.add(FeedbackDTO.newVisibleManualUnreferenced(
-                        i == 0 ? points.score() : 0.0, null, feedbackTexts.get(i)));
+                double score = points.score();
+                if (i != 0) {
+                    score = GLOBAL_FEEDBACK_OTHER_PARTS_SCORE;
+                }
+                feedbacks.add(FeedbackDTO.newVisibleManualUnreferenced(score, null, feedbackTexts.get(i)));
             }
             return feedbacks;
         }

src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/ComparatorUtils.java

@@ -4,30 +4,28 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
-import java.util.function.Function;
 
-final class ComparatorUtils {
+public final class ComparatorUtils {
     private ComparatorUtils() {}
 
     /**
-     * This is essentially {@link Comparator#comparing(Function)}, but for collections, which don't implement {@link Comparable} by default.
+     * Returns a comparator that compares collections based on their elements.
      * <p>
-     * Make sure that the provided collection has some kind of order, like a {@link java.util.TreeSet} or a {@link java.util.List}.
-     * The behavior is undefined if the collection is unordered.
+     * The comparator will first compare the elements element-wise, and if they are equal, the collection with fewer elements
+     * is considered smaller.
      *
-     * @param keyExtractor the function to extract the key from the object
-     * @return a comparator that compares the objects based on the extracted keys
-     * @param <T> the type of the objects to compare
-     * @param <V> the type of the keys to compare
+     * @param comparator the comparator to compare the elements with
+     * @return a comparator that compares collections based on their elements
+     * @param <T> the type of the elements in the collections
+     * @param <U> the type of the collections
      */
-    static <T, V extends Comparable<V>> Comparator<T> comparing(
-            Function<? super T, ? extends Collection<V>> keyExtractor) {
+    public static <T, U extends Collection<T>> Comparator<U> compareByElement(Comparator<? super T> comparator) {
         return (left, right) -> {
-            var leftList = new ArrayList<>(keyExtractor.apply(left));
-            var rightList = new ArrayList<>(keyExtractor.apply(right));
+            var leftList = new ArrayList<>(left);
+            var rightList = new ArrayList<>(right);
 
             for (int i = 0; i < Math.min(leftList.size(), rightList.size()); i++) {
-                int comparison = leftList.get(i).compareTo(rightList.get(i));
+                int comparison = comparator.compare(leftList.get(i), rightList.get(i));
                 if (comparison != 0) {
                     return comparison;
                 }
@@ -36,4 +34,21 @@ static <T, V extends Comparable<V>> Comparator<T> comparing(
             return Integer.compare(leftList.size(), rightList.size());
         };
     }
+
+    static <T extends Comparable<T>, U extends Collection<T>> Comparator<U> compareByElement() {
+        return compareByElement(Comparator.naturalOrder());
+    }
+
+    /**
+     * Returns a comparator that compares collections based on their size.
+     *
+     * @param comparator the comparator to compare collections with the same size
+     * @return a comparator that compares collections based on their size
+     * @param <T> the type of the elements in the collections
+     * @param <U> the type of the collections
+     */
+    public static <T, U extends Collection<T>> Comparator<U> shortestFirst(Comparator<? super U> comparator) {
+        // NOTE: does no longer compile if the lambda is replaced with Collection::size
+        return Comparator.comparingInt((U collection) -> collection.size()).thenComparing(comparator);
+    }
 }

src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/LocationFormatter.java

@@ -1,6 +1,7 @@
 /* Licensed under EPL-2.0 2025. */
 package edu.kit.kastel.sdq.artemis4j.grading.location;
 
+import java.util.Comparator;
 import java.util.List;
 import java.util.SequencedSet;
 import java.util.Set;
@@ -145,7 +146,8 @@ public int hashCode() {
     @Override
     public int compareTo(LocationFormatter other) {
         // Comparable is mostly implemented for convenience in intelligrade.
-        return ComparatorUtils.comparing(LocationFormatter::segments).compare(this, other);
+        return Comparator.comparing(LocationFormatter::segments, ComparatorUtils.compareByElement())
+                .compare(this, other);
     }
 
     /**

src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathFormatter.java

@@ -14,7 +14,6 @@ class PathFormatter {
     PathFormatter() {
         this.shouldMergeLines = false;
         this.showFilePath = true;
-
     }
 
     PathFormatter shouldMergeLines(boolean shouldMergeLines) {

src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathSegment.java

@@ -80,8 +80,8 @@ public int compareTo(PathSegment other) {
         //    ^ because the file does not have elements, it should be considered smaller
         // 3. if both are files, compare by locations
         return Comparator.comparing(PathSegment::name)
-                .thenComparing(ComparatorUtils.comparing(PathSegment::elements))
-                .thenComparing(ComparatorUtils.comparing(PathSegment::locations))
+                .thenComparing(PathSegment::elements, ComparatorUtils.compareByElement())
+                .thenComparing(PathSegment::locations, ComparatorUtils.compareByElement())
                 .compare(this, other);
     }