-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Multiroot for Report Viewer #569
Conversation
c5ce482
to
3416e17
Compare
bb3b4bb
to
2326c46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments :)
jplag/src/main/java/de/jplag/reporting/jsonfactory/DirectoryCreator.java
Outdated
Show resolved
Hide resolved
jplag/src/test/java/de/jplag/reporting/reportobject/mapper/ComparisonReportWriterTest.java
Outdated
Show resolved
Hide resolved
a8daeaa
to
33ad244
Compare
33ad244
to
5637c3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only had a quick look over the code, so there is a more in-depth review from @jplag/studdev required. But it tested the PR with two directories with 200 and 50 submissions, and everything worked fine.
Probably not related to this PR, but sometimes, the comparison viewer still breaks, but not predictable with the warning Could not find comparison file. indes.js:43
and then with the error:
templateLoader.js?e847:50
Uncaught (in promise) TypeError: _ctx.isAnonymous is not a function
at Proxy.render (templateLoader.js?e847:50:1)
at renderComponentRoot (runtime-core.esm-bundler.js?d2dd:896:1)
at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js?d2dd:5580:1)
at ReactiveEffect.run (reactivity.esm-bundler.js?89dc:185:1)
at instance.update (runtime-core.esm-bundler.js?d2dd:5694:1)
at setupRenderEffect (runtime-core.esm-bundler.js?d2dd:5708:1)
at mountComponent (runtime-core.esm-bundler.js?d2dd:5490:1)
at processComponent (runtime-core.esm-bundler.js?d2dd:5448:1)
at patch (runtime-core.esm-bundler.js?d2dd:5038:1)
at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js?d2dd:5660:1)
jplag/src/main/java/de/jplag/reporting/jsonfactory/ComparisonReportWriter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor things...
jplag/src/main/java/de/jplag/reporting/jsonfactory/ComparisonReportWriter.java
Outdated
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/jsonfactory/ComparisonReportWriter.java
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/jsonfactory/DirectoryCreator.java
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/reportobject/ReportObjectFactory.java
Outdated
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/reportobject/ReportObjectFactory.java
Outdated
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/reportobject/ReportObjectFactory.java
Outdated
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/reportobject/mapper/SubmissionNameToIdMapper.java
Show resolved
Hide resolved
jplag/src/main/java/de/jplag/reporting/reportobject/mapper/SubmissionNameToIdMapper.java
Outdated
Show resolved
Hide resolved
IDs for internal use, display name for displaying the submission name.
This reverts commit 7feae1e
b20837b
to
bcd843b
Compare
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! @nestabentum ready to merge from your end?
@tsaglam this is good to go |
Sorry, I'm late to the party, but I noticed that the |
Should be already fixed by #607 |
This is still WIP but I wanted to share my approach already to receive feedback
This PR intends to fix two major issues (both noted in issue #354):
A, A-B, C, B-C}
would result in twoA-B-C
)My approach to fixing this:
Submission name
simply refers to Submission#nameSubmission id
is newly introduced and used only in the context of the report viewer creation. Generation of this id is the topic of another (our next 😄 ) discussion, but currently, it is the sanitized version of the submission name (File separator replaced by underscore). This submission id serves as an id for internal handling in the report viewer.Display name
is the name of a submission to be displayed by the report viewer. Theoverview.json
file contains a map <submission id
->display name
> the report viewer will use to resolve the name for presentation purposes.overview.json
. It has the structure <submission id
-> <<submission id
> -><comparison file name
>>>. With this data structure, there will be no duplicate comparison file names and additionally, the comparison file name can be chosen arbitrarily as the frontend will only have to execute two gets two receive the file name and will not have to rely on string concatenation.Currently, the nested lookup table contains all submission ids in its first level, and in the second level, it contains each comparison for the corresponding submission id. This redundancy could be eliminated and actually halve the table's size but this is an optimization for later.
The UI isn't fully compatible with this approach yet, but will be with my next commits.
EDIT:
The report viewer is now fully compatible with the updated file format.
I realised that the problem in 2. persisted, as I still used id concatenation. Therefore, I changed the comparison file name generation to simply be a number that is counted up for each file. While this worsens human readability, it solves the aforementioned issue. As always, suggestions on how to solve this issue while maintaining readability are welcome.