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

patch-diff-report-tool: should preserve stylesheet.css from jxr site #424

Open
romani opened this issue Dec 15, 2019 · 6 comments
Open

patch-diff-report-tool: should preserve stylesheet.css from jxr site #424

romani opened this issue Dec 15, 2019 · 6 comments

Comments

@romani
Copy link
Member

romani commented Dec 15, 2019

We do read sources generated by JXR.
unfortunately tool does not copy it to diff folder, so sources become plain text.

customization of css:

          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-jxr-plugin</artifactId>
            <version>${jxr-plugin.version}</version>
            <!-- plugin do not have ability to define followinng by system property -->
            <configuration>
            	<stylesheet>${project.basedir}/jxr.css</stylesheet>
              <excludes>
                <exclude>**/.ci-temp/**/*</exclude>
              </excludes>
            </configuration>
          </plugin>

custom css, should use default + our customization for higlihting of target (anchored) line.

/* content of https://github.com/apache/maven-jxr/blob/master/maven-jxr-plugin/src/main/resources/stylesheet.css -- BEGIN */

body {
    background-color: #fff;
    font-family: Arial, Helvetica, sans-serif;
}

a:link {
    color: #00f;
}
a:visited {
    color: #00a;
}

a:active, a:hover {
    color: #f30 !important;
}

ul, li {
    list-style-type:none;
    margin:0;
    padding:0;
}

table td {
    padding: 3px;
    border: 1px solid #000;
}
table {
    width:100%;
    border: 1px solid #000;
    border-collapse: collapse;
}

div.overview {
    background-color:#ddd;
    padding: 4px 4px 4px 0;
}
div.overview li, div.framenoframe li {
    display: inline;
}
div.framenoframe {
    text-align: center;
    font-size: x-small;
}
div.framenoframe li {
    margin: 0 3px 0 3px;
}
div.overview li {
    margin:3px 3px 0 3px;
    padding: 4px;
}
li.selected {
    background-color:#888;
    color: #fff;
    font-weight: bold;
}

table.summary {
    margin-bottom: 20px;
}
table.summary td, table.summary th {
    font-weight: bold;
    text-align: left;
    padding: 3px;
}
table.summary th {
    background-color:#036;
    color: #fff;
}
table.summary td {
    background-color:#eee;
    border: 1px solid black;
}

em {
    color: #A00;
}
em.comment {
    color: #390;
}
.string {
    color: #009;
}

#overview {
    padding:2px;
}

hr {
    height: 1px;
    color: #000;
}

/* JXR style sheet */
.jxr_comment
{
    color: #390;
}

.jxr_javadoccomment
{
    color: #A00;
}

.jxr_string
{
    color: #009;
}

.jxr_keyword
{
    color: #000;
}
/* content of https://github.com/apache/maven-jxr/blob/master/maven-jxr-plugin/src/main/resources/stylesheet.css -- END */
/* BELOW ARE OUR CUSTOMIZATION */
:target {
  background-color: #b2b2e1;
}

to make it look like:
image

@rnveach
Copy link
Member

rnveach commented Dec 15, 2019

so sources become plain text.

jxr and maven-checkstyle-plugin inside checkstyle-tester should be removed as they just amount to nothing. Jxr just limits us to Java files and we can't run regression on non-java files. See #273 .

jxr generation of Java file for final report is done inside https://github.com/checkstyle/contribution/blob/master/patch-diff-report-tool/src/main/java/com/github/checkstyle/site/XrefGenerator.java#L84 and https://github.com/checkstyle/contribution/blob/master/patch-diff-report-tool/src/main/java/com/github/checkstyle/site/XrefGenerator.java#L142-L144 and that is where CSS would have to be injected. The only thing the tool lets you do is inject a footer.

http://rveach.no-ip.org/checkstyle/regression/reports/262/checkstyle/xref/src/it/resources/com/google/checkstyle/test/chapter4formatting/rule4842fallthrough/InputFallThrough.java.html
An example diff file does show that it is by default looking for a certain stylesheet.

<link type="text/css" rel="stylesheet" href="../../../../../../stylesheet.css" />

As I am not seeing this stylesheet anywhere, you could just add code to plop this file into the right directory and possibly all Java files would use it.

Going this route, however, means our custom TextTransform at

will also need to be modified to use the same stylesheet file as I don't believe it takes into account the directory structure like the Java one.

@rnveach
Copy link
Member

rnveach commented Jan 1, 2023

customization of css:
<configuration>
<stylesheet>${project.basedir}/jxr.css</stylesheet>

https://github.com/apache/maven-jxr/blob/6e0098709e4b5e392909638aabfed911b4b2f9a1/maven-jxr-plugin/src/main/java/org/apache/maven/plugin/jxr/AbstractJxrReport.java#L266
https://github.com/apache/maven-jxr/blob/6e0098709e4b5e392909638aabfed911b4b2f9a1/maven-jxr-plugin/src/main/java/org/apache/maven/plugin/jxr/AbstractJxrReport.java#L335
All this does is take the stylesheet we give it, and copies it to the required location.

It is not really a solution as what we need is for the utility to allow us to add more ../s or swap them with a global CSS URL in the HTML.

Example:
http://rveach.no-ip.org/checkstyle/regression/320/openjdk7/xref/test/java/lang/annotation/package-info.java.html
Needs the stylesheet placed into http://rveach.no-ip.org/checkstyle/regression/320/openjdk7/xref/test/java/stylesheet.css . As we can see, the folder is deeper than a global stylesheet would be for everything.

https://github.com/apache/maven-jxr/blob/15d6b3908b6340e127650dbc0a9fc2007d75beea/maven-jxr/src/main/java/org/apache/maven/jxr/JavaCodeTransform.java#L149
https://github.com/apache/maven-jxr/blob/15d6b3908b6340e127650dbc0a9fc2007d75beea/maven-jxr/src/main/java/org/apache/maven/jxr/JavaCodeTransform.java#L349-L351
The ../s added to the HTML for the stylesheet are the same count as the periods in the package file. There is no way to override the behavior as the methods are private. We cannot pass in any more parameters to the method to change the behavior.

It also looks to me, that this would even fully break apart when we do shorten file names for Windows users as it is still basing logic on package name and not the save location.

Our choices seem to be:

  1. Create our own JavaCodeTransform, duplicating theirs like we had to do for the TextTransform.
  2. Update the HTML file manually after it is created to swap the CSS.
  3. Put multiple stylesheets in all the locations they need to be in. Since this is based on the package and some projects have test classes with non-standard packages, we could be adding many.
  4. Submit a request and hope they change it and do a release (4 months since the last release, 1 month since the last code change)
  5. Drop this issue.

@romani ping

@romani
Copy link
Member Author

romani commented Jan 2, 2023

I do not want to depend on maven plugin.
I am ok with any approach to make line of code to be more easy to see.
'2' is good, '1' is also good.
Lets use what is more easy to implement, most likely "2", but if you feel to do "1" is also ok.

@nrmancuso
Copy link
Member

@rnveach would (2) help us to migrate from maven plugin?

@rnveach
Copy link
Member

rnveach commented Jan 2, 2023

I assume we are not referring to maven-checkstyle-plugin, as that has no bearing here for patch-diff-report.

If we are referring to the JXR plugin which generates the Java HTML file for us, then to get away from it we have to drop the dependency completely. This means option 1 is the only solution as all others rely on the JXR plugin to do the work for us.

However, to completely drop JXR requires us to duplicate many core files. I don't know how many there will be or how much there will be but it does NOT seem like this is the easiest solution.

@romani
Copy link
Member Author

romani commented Jan 2, 2023

Let's use easy solution for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants