TheCoder4eu / BootsFaces-OSP

BootsFaces - Open Source Project
Apache License 2.0
246 stars 102 forks source link

<b:dataTable> breaks when using rendered on a column #848

Closed RuslanVNikolov closed 6 years ago

RuslanVNikolov commented 7 years ago

Currently, I've noticed that when using rendered attribute on a or a this breaks the page with the following error: TypeError: i is undefined[Learn More] datatables.min.js.jsf:36:271 This causes the table's design to break. Is there a way to do hide a column without this happening? Maybe something I'm not using properly?

stephanrauh commented 7 years ago

Like so?

<b:dataTable value="#{carPool.carPool}" 
              var="car"
              widgetVar="myDataTableWidgetVar">
     <b:dataTableColumn value="#{car.brand}" />
     <b:dataTableColumn value="#{car.type}" rendered="false"/>
     <b:dataTableColumn value="#{car.color}" rendered="false"/>
     <b:dataTableColumn value="#{car.year}"  order="asc" rendered="false" />
     <b:dataTableColumn value="#{car.price}"  rendered="false"/>
     <b:dataTableColumn value="#{car.enginePower}" label="Engine Power (hp)" rendered="false" />
</b:dataTable>

It works fine on my machine. Only when I set rendered="false" on every column, there's an error message. Even so, it a different error message.

So what does your code look like exactly?

stephanrauh commented 7 years ago

@RuslanVNikolov I'll close the ticket now because I can't reproduce it. If you send us a reproducer, we'll happily have a look at it. It's just that I don't have a clue how to proceed because my tests didn't show any problems.

vsvetoslavov commented 7 years ago

Hi I confirm we have this problem (plus I work together with Ruslan). I did a little bit of investigation and noticed that there is a column list (something like columns: [null, null, null, ..., {orderable: false}] ) inside the generated javascript. Further investigation led me to the following lines inside DataTableRenderer class:

String orderable = column.getAttributes().get("orderable").toString();
if ("false".equalsIgnoreCase(orderable)) {
    if (dataTable.getColumnInfo() == null) {
        List<String> infos = new ArrayList<String>(dataTable.getChildren().size());
        for (int k = 0; k < dataTable.getChildren().size(); k++) {
            infos.add(null);
        }
        dataTable.setColumnInfo(infos);
    }
    List<String> infos = dataTable.getColumnInfo();
    String s = infos.get(index);
    if (s == null) {
        infos.set(index, "'orderable': false");
    } else
        infos.set(index, s + ",'orderable': false");
}

As you may notice, the array list size ignores the rendered attribute - which leads to a JS error. To confirm this finding I removed the orderable attribute of the only column that had it (the rest use the defaults for sorting) - when there was no columns definition, there was no JS error. @stephanrauh The sample you wrote above will not reproduce the problem, because you have no rendered column that has orderable="false" | dataType | customOptions | orderBy attribute. You can test with a configuration that has several columns, one with orderable="false" and another with rendered="false". I guess this behaviour might be observed also in all the above cases... We had the rendered property bound to a value from the controller, but I did my testing with static values and it still did break the datatable. I could pull the code and do the refactoring for this, but I'm not really sure what the correct workflow is for that, and how to do a "pull request" or whatever, so I'd rather wait, but if someone has the time to explain to me these details, I'll gladly contribute... :)

ggam commented 7 years ago

Hi @vsvetoslavov unfortunately I don't have time right now to look further at it, but I think you could add a check in https://github.com/TheCoder4eu/BootsFaces-OSP/blob/cf801c16299be58b645f2fc7e1958b433e99467e/src/main/java/net/bootsfaces/component/dataTable/DataTableRenderer.java#L479 for "false".equalsIgnoreCase(column.getAttributes().get("rendered").toString()) and it should be fine.

Would you be able to test if that works? In case that resolves your bug, you could just send a PR with the changed code so we can easily test also on our machines. No CLA or special protocol is needed.

ggam commented 7 years ago

Also, while at it, could you please check if other properties could be affected by the same bug?

vsvetoslavov commented 7 years ago

@ggam As I mentioned before there are several places inside the big for loop,that use the children size:

ggam commented 7 years ago

@vsvetoslavov Sorry I didn't read your previous post thoroughly.

A private method on the DataTableRenderer#getRenderedColumnsCount(DataTable) would be fine I guess. Also keep in mind that the getChildren() method returns every child UIComponent, which could include other (probably erroneous) components placed there by the user. Those would also need to be filtered out.

vsvetoslavov commented 7 years ago

DataTableRenderer.zip Sorry, I'm really not used to git, so I am not sure whether to commit, or to push upstream, or... So I have uploaded the file in this comment. Can someone do the git stuff for me, please? Also, I haven't tested it yet, only checked that it compiles without errors.

ggam commented 7 years ago

Don't worry. GitHub provides a nice UI to edit files and make commits and PRs from the web. If you go to https://github.com/TheCoder4eu/BootsFaces-OSP/blob/cf801c16299be58b645f2fc7e1958b433e99467e/src/main/java/net/bootsfaces/component/dataTable/DataTableRenderer.java you'll see a pencil icon on the top right. Clicking there will fork the repository on your account (like doing a git clone) and then you'll be able to edit. You can paste your changes there and GitHub will commit+push for you. After that, you'll be prompted to create a pull request. You can still make changes on your branch after doing the PR and changes will be reflected there.

If you do it that way we'll be able to merge and maintain your name as the author

stephanrauh commented 7 years ago

@vsvetoslavov It would be very nice of you to follow the instructions of Guillermo (aka ggam). That'd make our lives easier. But what's far more important: thanks for contributing to BootsFaces!

ggam commented 7 years ago

Sorry @vsvetoslavov I forgot to say thank you too :)

On mié., 27 de septiembre de 2017 21:50 Stephan Rauh < notifications@github.com> wrote:

@vsvetoslavov https://github.com/vsvetoslavov It would be very nice of you to follow the instructions of Guillermo (aka ggam). That'd make our lives easier. But what's far more important: thanks for contributing to BootsFaces!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/TheCoder4eu/BootsFaces-OSP/issues/848#issuecomment-332635659, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAucNAasPiGsV2eetXRMTP-bA1yBiCgks5smqbsgaJpZM4PCrmc .

vsvetoslavov commented 7 years ago

@ggam, @stephanrauh Thanks for the help guys. I really hope I helped improve (a little bit) a library that I use ;) Meanwhile, I noticed that labelHasBeenRendered also ignores whether a column is rendered or not, but I wasn't sure about the logic there, so maybe someone could look into that.

vsvetoslavov commented 7 years ago

Could someone please remove the "Waiting for user" label? I think it's outdated by now.

stephanrauh commented 6 years ago

I guess we can close this ticket now :). This time for real!