Skip to content
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

Fix Report Viewer #573

Merged
merged 3 commits into from
Aug 9, 2022
Merged

Fix Report Viewer #573

merged 3 commits into from
Aug 9, 2022

Conversation

nestabentum
Copy link
Contributor

@nestabentum nestabentum commented Aug 7, 2022

Concerning issue #571 :

I falsely assumed that the submission file name would always have the same index when splitting its path, as this was always the case when I tested the viewer. The submission files in my zip always had the path result/submissions/<submission-file>, therefore I extracted the submission file name from splitting the path at / and accessing index 2.
The submission files in the zip you provided have the path submission/<submission-file> and therefore the filenames are not extracted correctly.

Consequently, I changed the file name extraction to locate the index of the submission folder and find the submission files by a accessing the the found index+1.

Additionally, I changed the way how empty submissions are displayed: Instead of showing one empty line, the text "Empty File" is displayed.
Screenshot 2022-08-07 at 12 48 27

@nestabentum nestabentum mentioned this pull request Aug 7, 2022
@sebinside sebinside self-requested a review August 7, 2022 12:30
@sebinside
Copy link
Member

Thank you for your fast reaction, I will review the PR tomorrow.

@sebinside sebinside added minor Minor issue/feature/contribution/change report-viewer PR / Issue deals (partly) with the report viewer and thus involves web-dev technologies labels Aug 7, 2022
@tsaglam tsaglam added the bug Issue/PR that involves a bug label Aug 7, 2022
@tsaglam tsaglam added this to the v4.0.0 milestone Aug 7, 2022
Copy link
Member

@sebinside sebinside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is resolved, thank you. However, the Empty file was not displayed in my tests.
image

@sebinside
Copy link
Member

@nestabentum @tsaglam Also, not related to this issue, but also broken: Didn't an earlier version of the report viewer highlight plagiarized code snippets in the viewer? At least in my tests today, this feature was not visible. Or did I miss something?

@sebinside
Copy link
Member

Ah, and I think the lines are also broken here 😅
image

@nestabentum
Copy link
Contributor Author

The issue is resolved, thank you. However, the Empty file was not displayed in my tests.

@sebinside Oh, I forgot to push the corresponding commit. It should work now.

@nestabentum
Copy link
Contributor Author

nestabentum @tsaglam Also, not related to this issue, but also broken: Didn't an earlier version of the report viewer highlight plagiarized code snippets in the viewer? At least in my tests today, this feature was not visible. Or did I miss something?

@sebinside both this issue and the weird looking side panel stem from a bug in the match computation in the report generation. It happens in ComparisonReportMapper#convertMatchToReportMatch. This was last affected by PR #552.

The match computation was dependent on Language#usesIndex which has been replaced by Language#supportsColumns. When the value of the aforementioned methods is true, the match indices are fetched from Token#getIndex, otherwise they are fetched from Token#getLine.
Pretty much all implemented Languages seem to supportColumns and therefore access Token#getIndex which returns -1 (except CharToken). This is the reason for the missing highlighting and -1 being everywhere in the side panel.

I don't really get why a differentiation based on Language#supportsColumns (or formerly Language#usesIndex) is necessary in the first place and why it isn't enough to just use Token#getLine. @tsaglam and @sebinside can you give me a hint concerning this?

@tsaglam
Copy link
Member

tsaglam commented Aug 8, 2022

The match computation was dependent on Language#usesIndex which has been replaced by Language#supportsColumns. When the value of the aforementioned methods is true, the match indices are fetched from Token#getIndex, otherwise they are fetched from Token#getLine.

Pretty much all implemented Languages seem to supportColumns and therefore access Token#getIndex which returns -1 (except CharToken). This is the reason for the missing highlighting and -1 being everywhere in the side panel.

I don't really get why a differentiation based on Language#supportsColumns (or formerly Language#usesIndex) is necessary in the first place and why it isn't enough to just use Token#getLine. @tsaglam and @sebinside can you give me a hint concerning this?

Careful here, basically usesIndex() == !supportsColumns(). If either of those is true, the other one is false. This is why I removed usesIndex. What do these methods mean? Most languages proved line and column indices for a token, meaning in which line in the source code the token is and where in the line. However, some frontends do not. They only provide a continuous index for the code. For example, the character frontend only tells you that the token is the nth character, but has no concept of lines and columns. Historically, the were more frontends like this, but I adapted them to support line and column indices.

What does that mean for you? If the frontend has supportsColumns() == true you use the line and column indices via getLine() and getColumn() (probably just the line index, right?). If it returns false you use the continuous index getIndex().

So change this:

.map(match -> convertMatchToReportMatch(comparison, match, result.getOptions().getLanguage().supportsColumns())).toList();

to !result.getOptions().getLanguage().supportsColumns() (meaning ¬ supportsColumns())

EDIT: As you are confused by the continuous index, it could mean the report viewer does not really support the continuous index and just uses it as a line index? We can check this by using the char frontend and checking if the report displays it correctly (after fixing the line above of course).

@nestabentum
Copy link
Contributor Author

@tsaglam thanks for the explanation, now I get it. I changed the code accordingly. The ComparisonView should now be back to its old beauty
Screenshot 2022-08-08 at 18 55 08

EDIT: As you are confused by the continuous index, it could mean the report viewer does not really support the continuous index and just uses it as a line index? We can check this by using the char frontend and checking if the report displays it correctly (after fixing the line above of course).

Yes, currently, the report viewer only supports line-wise highlighting.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@tsaglam
Copy link
Member

tsaglam commented Aug 8, 2022

The ComparisonView should now be back to its old beauty

Great to hear!

Yes, currently, the report viewer only supports line-wise highlighting.

@sebinside we should not address this, we should just remove the char frontend.

@sebinside
Copy link
Member

@nestabentum Thank you for the fast work. Really appreciated!
The web viewer works again and in a smaller example, the highlighting was fine. In a large one, the highlighting is still not displayed (although the line numbers are now there). As I currently don't know the origin of this problem, I'll add the problem back into issue #357 and merge this PR for now. Thank you!

@sebinside sebinside merged commit fb738b8 into jplag:master Aug 9, 2022
@sebinside sebinside mentioned this pull request Aug 9, 2022
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue/PR that involves a bug minor Minor issue/feature/contribution/change report-viewer PR / Issue deals (partly) with the report viewer and thus involves web-dev technologies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants