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

the filter & server mode now play nicely with the colReorder extension #532

Merged
merged 8 commits into from Apr 25, 2018

Conversation

shrektan
Copy link
Collaborator

@shrektan shrektan commented Apr 22, 2018

Closes #531

Description

Before this PR, if the user enables the colReorder extension, the filter will return wrong results in the client mode. In the server processing mode, the table will be reverted to the original order whenever it gets re-drawed (i.e., triggered by the filter, search or the paginate button)

The causes

  • Client Mode: The row data returned by the datatable is reordered but the filter functions itself is bind to the original order. So the wrong data gets filtered;
  • Server Mode: Since the data processing is done in R, the row data is not reordered. However, because R doesn't know the columns have been reordered, it returns the data with column unchanged.

After this PR

It should work for both cases.

The example

library(shiny)
library(DT)

ui <- fluidPage(
  h1("server - FALSE"),
  fluidRow(
    column(
      6,
      DT::dataTableOutput('tbl_with_colreorder')
    ),
    column(
      6,
      DT::dataTableOutput('tbl_wo_colreorder')
    )
  ),
  h1("server - TRUE"),
  fluidRow(
    column(
      6,
      DT::dataTableOutput('tbl_with_colreorder_s')
    ),
    column(
      6,
      DT::dataTableOutput('tbl_wo_colreorder_s')
    )
  )
)

server <- function(input, output, session) {
  output$tbl_with_colreorder = DT::renderDataTable(
    iris, server = FALSE,
    filter = "top", 
    caption = 'with ColReorder',
    extensions = 'ColReorder',
    options = list(colReorder = TRUE)
  )
  output$tbl_wo_colreorder = DT::renderDataTable(
    iris, server = FALSE,
    filter = "top", 
    caption = 'w/o ColReorder'
  )
  output$tbl_with_colreorder_s = DT::renderDataTable(
    iris, server = TRUE,
    filter = "top", 
    caption = 'with ColReorder',
    extensions = 'ColReorder',
    options = list(colReorder = TRUE)
  )
  output$tbl_wo_colreorder_s = DT::renderDataTable(
    iris, server = TRUE,
    filter = "top", 
    caption = 'w/o ColReorder'
  )
}

runApp(shinyApp(ui, server))

@yihui yihui added this to the v0.5 milestone Apr 24, 2018
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

This is great! Thanks!

// the table.colReorder.order() function will exist but throws error when called.
// So it seems like the only way to know if colReorder is enabled or not is to
// check the options.
var colReorderEnabled = function() { return "colReorder" in options; };
Copy link
Member

Choose a reason for hiding this comment

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

Does it have to be a function? I mean, does this work?

var colReorderEnabled = "colReorder" in options;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I used the non-function version at first and worked fine...

However, I overthinked that the options may get changed somewhere else after the table being initialized... It's OK to change it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It should not matter much. I'm fine to use a function here.

for (i = 0; i < data.length; ++i) {
row = data[i].slice();
for (j = 0; j < order.length; ++j) data[i][j] = row[order[j]];
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there is a way to reorder the data on the server side. If there is, the implementation could be much easier (data[order] in R instead of a nested loop in JS). Of course, it depends on whether we have the order information on the server side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but the datatables library doesn't send the reordered column info to the server by default. We need to register a callback in the event column-reorder, which sends the info to the R side. If this method is better, I guess it's not very difficult to make the change.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, let's just forget it. I'm not too concerned about the performance of the nested loops here since data should typically be small enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the concern too... For the deep copy and nested loops... but as you said, it should be fine in the most cases.

@yihui yihui mentioned this pull request Apr 24, 2018
1 task
@yihui yihui merged commit fce32f8 into rstudio:master Apr 25, 2018
@yihui
Copy link
Member

yihui commented Apr 25, 2018

Just a reminder to myself -- I need to update the docs at https://rstudio.github.io/DT/extensions.html

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

Successfully merging this pull request may close these issues.

None yet

2 participants