From ac82b5cf2e0c7bddd767123f55eb97ab04e0a0c7 Mon Sep 17 00:00:00 2001 From: Luro02 <24826124+Luro02@users.noreply.github.com> Date: Sun, 9 Feb 2025 11:25:26 +0100 Subject: [PATCH] improve global feedback display #159 --- .../sdq/artemis4j/grading/Assessment.java | 164 +++++++++++++----- .../grading/location/ComparatorUtils.java | 43 +++-- .../grading/location/LocationFormatter.java | 4 +- .../grading/location/PathFormatter.java | 1 - .../grading/location/PathSegment.java | 4 +- .../kit/kastel/sdq/artemis4j/End2EndTest.java | 96 ++++++++++ 6 files changed, 254 insertions(+), 58 deletions(-) diff --git a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/Assessment.java b/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/Assessment.java index 798acb4..8c5f0a6 100644 --- a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/Assessment.java +++ b/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. + *

+ * 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. + *

+ * 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 annotations; private final List testResults; @@ -123,7 +145,11 @@ public List getAnnotations() { * @return An unmodifiable list of annotations, possibly empty but never null. */ public List getAnnotations(MistakeType mistakeType) { - return this.annotations.stream() + return this.getAnnotations(mistakeType, this.annotations); + } + + private List getAnnotations(MistakeType mistakeType, Collection 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 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 packAssessmentForArtemis() throws AnnotationMappingExc return feedbacks; } - private FeedbackDTO createInlineFeedback(Map.Entry> annotations) { - var sampleAnnotation = annotations.getValue().get(0); + private FeedbackDTO createInlineFeedback(Map.Entry> annotations) { + var sampleAnnotation = annotations.getValue().getFirst(); String text = "File " + sampleAnnotation.getFilePathWithoutType() + " at line " + sampleAnnotation.getDisplayLine(); @@ -511,6 +532,40 @@ private FeedbackDTO createInlineFeedback(Map.Entry> an return FeedbackDTO.newManual(0.0, text, reference, detailText); } + private TranslatableString formatGlobalFeedbackAnnotations(List 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 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 lines = new ArrayList<>(); + // group annotations by mistake type + List>> 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 lines = new ArrayList<>(); + for (var entry : annotationsByType) { + MistakeType mistakeType = entry.getKey(); + List annotationsWithType = entry.getValue(); + Optional 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 '* at line (P)' - // Otherwise, it's just '* at 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 hasScore = a -> a.getCustomScore().isPresent() && ratingGroup.isScoringGroup(); + + // separate annotations with and without score + List annotationsWithScore = + annotations.stream().filter(hasScore).toList(); + List annotationsWithoutScore = + annotations.stream().filter(Predicate.not(hasScore)).toList(); + + // For custom annotations, we have '* at line (P)' + if (!annotationsWithScore.isEmpty()) { + lines.add(this.formatGlobalFeedbackAnnotations(annotationsWithScore, true)); + } + + // Otherwise, it's just '* at line ' + // Lines are zero-indexed + if (!annotationsWithoutScore.isEmpty()) { + lines.add(this.formatGlobalFeedbackAnnotations(annotationsWithoutScore, false)); } } } @@ -580,8 +661,11 @@ private List createGlobalFeedback(RatingGroup ratingGroup) { List 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; } diff --git a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/ComparatorUtils.java b/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/ComparatorUtils.java index 85ef859..c61026d 100644 --- a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/ComparatorUtils.java +++ b/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. *

- * 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 the type of the objects to compare - * @param 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 the type of the elements in the collections + * @param the type of the collections */ - static > Comparator comparing( - Function> keyExtractor) { + public static > Comparator compareByElement(Comparator 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 > Comparator comparing( return Integer.compare(leftList.size(), rightList.size()); }; } + + static , U extends Collection> Comparator 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 the type of the elements in the collections + * @param the type of the collections + */ + public static > Comparator shortestFirst(Comparator comparator) { + // NOTE: does no longer compile if the lambda is replaced with Collection::size + return Comparator.comparingInt((U collection) -> collection.size()).thenComparing(comparator); + } } diff --git a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/LocationFormatter.java b/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/LocationFormatter.java index 4698625..0eb5926 100644 --- a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/LocationFormatter.java +++ b/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); } /** diff --git a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathFormatter.java b/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathFormatter.java index 79bbaeb..0d6e0e3 100644 --- a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathFormatter.java +++ b/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) { diff --git a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathSegment.java b/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathSegment.java index 6862500..ec6542e 100644 --- a/src/main/java/edu/kit/kastel/sdq/artemis4j/grading/location/PathSegment.java +++ b/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); } diff --git a/src/test/java/edu/kit/kastel/sdq/artemis4j/End2EndTest.java b/src/test/java/edu/kit/kastel/sdq/artemis4j/End2EndTest.java index 761b714..7266b21 100644 --- a/src/test/java/edu/kit/kastel/sdq/artemis4j/End2EndTest.java +++ b/src/test/java/edu/kit/kastel/sdq/artemis4j/End2EndTest.java @@ -7,6 +7,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Optional; @@ -361,4 +363,98 @@ void testHighlight() { MistakeType.Highlight.NONE, this.gradingConfig.getMistakeTypeById("magicLiteral").getHighlight()); } + + @Test + void testGlobalFeedbackSorting() throws ArtemisClientException { + // First add some annotations + String defaultFeedbackText = "Some feedback text"; + + for (int i = 0; i < 5; i++) { + this.assessment.addPredefinedAnnotation( + this.gradingConfig.getMistakeTypeById("jdEmpty"), + new Location("src/edu/kit/informatik/BubbleSort.java", i, i), + defaultFeedbackText); + } + + this.assessment.addPredefinedAnnotation( + this.gradingConfig.getMistakeTypeById("jdEmpty"), + new Location("src/edu/kit/informatik/InsertionSort.java", 5, 5), + defaultFeedbackText); + + for (int i = 0; i < 3; i++) { + this.assessment.addPredefinedAnnotation( + this.gradingConfig.getMistakeTypeById("jdEmpty"), + new Location("src/edu/kit/informatik/RadixSort.java", i + 3, i + 5), + defaultFeedbackText); + } + + // to test custom scoring, add some custom annotations: + for (int i = 0; i < 3; i++) { + this.assessment.addCustomAnnotation( + this.gradingConfig.getMistakeTypeById("custom"), + new Location("src/edu/kit/informatik/RadixSort.java", i, i), + defaultFeedbackText, + -0.5); + } + + this.assessment.addCustomAnnotation( + this.gradingConfig.getMistakeTypeById("custom"), + new Location("src/edu/kit/informatik/InsertionSort.java", 5, 5), + defaultFeedbackText, + 0.0); + + for (int i = 0; i < 2; i++) { + this.assessment.addCustomAnnotation( + this.gradingConfig.getMistakeTypeById("custom"), + new Location("src/edu/kit/informatik/BubbleSort.java", i, i), + defaultFeedbackText, + 0.0); + } + + this.assessment.addCustomAnnotation( + this.gradingConfig.getMistakeTypeById("custom"), + new Location("src/edu/kit/informatik/BubbleSort.java", 3, 3), + defaultFeedbackText, + -0.5); + + // this should be the first annotation + this.assessment.addPredefinedAnnotation( + this.gradingConfig.getMistakeTypeById("jdTrivial"), + new Location("src/edu/kit/informatik/BubbleSort.java", 0, 0), + defaultFeedbackText); + + this.assessment.submit(); + this.assessment = this.programmingSubmission.tryLock(this.gradingConfig).orElseThrow(); + + ResultDTO resultDTO = this.programmingSubmission.getRelevantResult().orElseThrow(); + var feedbacks = ResultDTO.fetchDetailedFeedbacks( + this.connection.getClient(), + resultDTO.id(), + this.programmingSubmission.getParticipationId(), + resultDTO.feedbacks()); + + Collection globalFeedbackLines = new ArrayList<>(); + for (FeedbackDTO feedbackDTO : feedbacks) { + if (feedbackDTO.type() != FeedbackType.MANUAL_UNREFERENCED || feedbackDTO.visibility() != null) { + continue; + } + + globalFeedbackLines.addAll(Arrays.asList(feedbackDTO.detailText().split("\\n"))); + } + + assertEquals( + List.of( + "Funktionalität [-12P (Range: -20P -- ∞P)]", + " * JavaDoc Trivial [-5P]:", + " * src/edu/kit/informatik/BubbleSort.java at line 1", + " * Custom Penalty [-2P]:", + " * src/edu/kit/informatik/RadixSort.java at lines 1, 2, 3 (-1,5P)", + " * src/edu/kit/informatik/InsertionSort.java at line 6 (0P)", + " * src/edu/kit/informatik/BubbleSort.java at lines 1, 2, 4 (-0,5P)", + " * JavaDoc Leer [-5P]:", + " * src/edu/kit/informatik/BubbleSort.java at lines 1, 2, 3, 4, 5", + " * src/edu/kit/informatik/InsertionSort.java at line 6", + " * src/edu/kit/informatik/RadixSort.java at lines 4, 5, 6"), + globalFeedbackLines); + } }