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

support shift+click to select mutiple rows #475

Merged
merged 2 commits into from Jan 15, 2018
Merged

Conversation

@shrektan
Copy link
Collaborator

@shrektan shrektan commented Jan 7, 2018

closes #305

@shrektan
Copy link
Collaborator Author

@shrektan shrektan commented Jan 7, 2018

Here's my testing code.

library(shiny)

server <- function(input, output) {
  output$data <- DT::renderDT(
    DT::datatable(iris), server = F
  )
  output$data_sel <- renderText(input$data_rows_selected)
}
ui <- fluidPage(
  tagList(
    DT::DTOutput("data"),
    textOutput("data_sel")
  )
)

shinyApp(ui, server, options = list(port = 7995))
closes #305
@shrektan shrektan force-pushed the shrektan:shift-sel branch from 1ef2ee9 to e4b12ad Jan 8, 2018
@yihui yihui mentioned this pull request Jan 10, 2018
3 of 3 tasks complete
@yihui yihui added this to the v0.3 milestone Jan 15, 2018
@yihui
yihui approved these changes Jan 15, 2018
Copy link
Member

@yihui yihui left a comment

Overall this looks great! We need to prevent the text selection when holding Shift. I'll merge it and see if I can change it (presumably using https://api.jquery.com/event.preventdefault/).

@yihui yihui merged commit b307cbd into rstudio:master Jan 15, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yihui
Copy link
Member

@yihui yihui commented Jan 15, 2018

I have fixed the text selection issue. The other thing is that this probably does not work in the server-processing mode (which is always tricky in DT)...

@shrektan
Copy link
Collaborator Author

@shrektan shrektan commented Jan 16, 2018

Well, just confirm that there's indeed a small issue (not that small...), which is the selection range will be different between the server-processing mode and the client side mode.

  1. select first row in page 1, let's assume it's row no 1;
  2. press shift
  3. select another row in another page, let's assume it's row no 20, and every page displays 5 row

In the client side mode, the row selection will be 1 to 20, while in the server-processing mode, it's 1, 16 to 20...

@yihui
Copy link
Member

@yihui yihui commented Jan 16, 2018

You need to use the vector DT_rows_current to convert the local row index to the index on the server side. You can see how I did it in changeInput('rows_selected') and changeInput('row_last_clicked'). Let me know if you are able to fix it. Thanks!

@shrektan
Copy link
Collaborator Author

@shrektan shrektan commented Jan 16, 2018

I guess I should be able to fix it based on your current code, some ideas on server-processing mode:

  • lastClickedRow and currentClickedRow (i.e., thisRow.index()) should be converted to the index on server side;
  • shiftSelRows() should return the indexes on server side as well. Since on client side, the browser itself doesn't know what the real underlying data is, we can only convert the indexes relying on DT_rows_current.
  • Select the rows based on server side indexes. Here's the tricky part, how can I be able to select rows that hasn't been loaded to the browsers yet? By looking at your implementation for selection = list(mode = "multiple", selected = c(1, 3, 8) I think it can be done by changing the value of selected1.

Will try to fix it tonight or tomorrow~

@yihui
Copy link
Member

@yihui yihui commented Jan 16, 2018

Sounds great!

@yihui
Copy link
Member

@yihui yihui commented Jan 17, 2018

我的 0.3 版本计划的事项现在还剩下两项:https://github.com/rstudio/DT/issues?q=is%3Aopen+is%3Aissue+milestone%3Av0.3 我先做 #28 去了,你这个事项需要比较细心的考虑,所以你慢慢来不着急,下周之前能捣鼓出来最好,捣鼓不出来也没事。多谢!

@shrektan
Copy link
Collaborator Author

@shrektan shrektan commented Jan 17, 2018

恩恩,白天上班没什么时间 👨‍💼

昨天稍微鼓捣了下,要是没有幺蛾子的话,感觉应该可以弄出来。因为没有测试嘛,代码有无其他坏的影响,姑且只能由你来把关啦,哈哈哈。

@yihui
Copy link
Member

@yihui yihui commented Jan 17, 2018

嗯,我也只能用意念力在脑子里“测试”。

@shrektan shrektan deleted the shrektan:shift-sel branch Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants