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

Resize column widget on PaginatedDataTable2 #93

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

nerdknight
Copy link

Widget to resize columns manually. It can be anabled by setting to true the resizeable variable in the DataColumn2 constructor.
https://user-images.githubusercontent.com/5269947/169674825-17f5f538-7c22-417e-8b41-297605912657.mov

Santiago Conti added 2 commits May 20, 2022 20:28
- Changed icons on Draggable widget
- Now the columns move with the mouse pointer
@maxim-saplin
Copy link
Owner

Thanks for submitting the PR, the feature seems great. Though there're a few issues:

  1. There're failing unit test (try running them locally via VSCode)
  2. There's an exception when resizing window size

image

  1. There's still lag/misallignment when dragging and resizing a row. Might there be an old version in PR? Tested in Chrome and as macOS desktop app

JIC, here's my flutter doctor -v output

[✓] Flutter (Channel stable, 3.0.1, on macOS 12.3.1 21E258 darwin-x64, locale en-GB)
• Flutter version 3.0.1 at /private/var/user/flutter
• Upstream repository https://github.com/flutter/flutter.git
• Framework revision fb57da5f94 (3 days ago), 2022-05-19 15:50:29 -0700
• Engine revision caaafc5604
• Dart version 2.17.1
• DevTools version 2.12.2

[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
• Android SDK at /var/user/Library/Android/sdk
• Platform android-31, build-tools 31.0.0
• ANDROID_HOME = /var/user/Library/Android/sdk
• Java binary at: /Applications/Android Studio 4.1 Preview.app/Contents/jre/Contents/Home/bin/java
• Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)
• All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.4)
• Xcode at /Applications/Xcode.app/Contents/Developer
• CocoaPods version 1.11.3

[✓] Chrome - develop for the web
• CHROME_EXECUTABLE = /Applications/chrome_no_cors.sh

[✓] Android Studio (version 2020.3)
• Android Studio at /Applications/Android Studio 4.1 Preview.app/Contents
• Flutter plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
🔨 https://plugins.jetbrains.com/plugin/6351-dart
• Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)

[✓] VS Code (version 1.67.2)
• VS Code at /Applications/Visual Studio Code.app/Contents
• Flutter extension version 3.40.0

[✓] Connected device (2 available)
• macOS (desktop) • macos • darwin-x64 • macOS 12.3.1 21E258 darwin-x64
• Chrome (web) • chrome • web-javascript • Google Chrome 101.0.4951.64

[✓] HTTP Host Availability
• All required HTTP hosts are available

@nerdknight
Copy link
Author

Fixed the failing unit tests and the exception when resizing the window.
I notice the lag/missaligment problem only if resizing too fast or when I try to shrink the column below the minimum (in the later case I think there is nothing we can do).
If I resize the column not too fast, there doesn't seem to be missaligments. I'll try to find a solution for this, let me know if you have a clue.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Attention: Patch coverage is 72.54335% with 95 lines in your changes are missing coverage. Please review.

Project coverage is 88.97%. Comparing base (1919e3d) to head (26e387e).

Current head 26e387e differs from pull request most recent head 58570cd

Please upload reports for the commit 58570cd to get more accurate results.

Files Patch % Lines
lib/src/data_table_2_resizable.dart 20.21% 75 Missing ⚠️
lib/src/data_table_2.dart 92.06% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
- Coverage   95.87%   88.97%   -6.91%     
==========================================
  Files           3        4       +1     
  Lines        1140     1197      +57     
==========================================
- Hits         1093     1065      -28     
- Misses         47      132      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maxim-saplin
Copy link
Owner

maxim-saplin commented May 23, 2022

Added a sample for DataTable2, please note that thus sample has minWidth set. In that case resizing is not working

image

@maxim-saplin
Copy link
Owner

By default DataTable2 expands to it's container, it's width is determined by the number of pixel LayoutBuilder provides. I.e. the default scenario is that DataTable2 doesn't have a fixed width, should anyone want a fixed width, the control must be wrapped in limmiting widget.

For the cases when viewport becomes to narrow and Columns get condensed to non-reasonable widths there's a minWidth property which stops shrinking

Speaking of resizing columns expected behaviour:

  1. By default no total width of the table is changed, extending or shrinking a column is achieved via redistributing available width
  2. [Optional] we can consider adding a feature to let darg rightmost column and get total width changes to the DataTable2

Here're some thoughts which I'm not quite sure yet in terms of implementability/consictency, your considerations are welcome:

  • It is a common practice to resize column when clicking at column border, changing cursor to resize - idiomatic for Desktop
  • Cursors are not available at touch devices, in that case we need that dragable icon
  • We might need some way of controling when to show the dragable widget or not - either let developers control that behavior or present some helper to make the choise of Dekstop vs Touch -> the general tradeoff of explicit/predicatble vs implicit/handy in default cases
  • When changing column width we can either make it fixed width (and it will stay fixed when table width changes) or we can adjust ratio for that column so that it changes proportionaly to table width change
  • When implementing width change via border mouse down, we either treat the colukn to the left being the subject of width change. Or it migh require a more complicated approach when both columns must be treated as subjected to change

@maxim-saplin
Copy link
Owner

One more thing, DataTable2 is stateless while PaginatedDataTable2 is statefull, apparently dragging columns will require to adress that issue cause it might be to reasonable to have that behavior implemented in both widget. Yet making DataTable2 stateful might be a poor option cause it is treated as direct replacement to Flutter's DataTable which is statless - not usre if switching DataTable2 to staefull might present any challenges in this regard

@nerdknight
Copy link
Author

By default no total width of the table is changed, extending or shrinking a column is achieved via redistributing available width

I don't see any problem resizing the total width of the table when resizing a column, it's not uncommon. In other platforms that implement resizing columns it works this way, and I think if you want resizable columns you should expect variable table width. And if, for example, all the columns are defined with fixed width, I don't see another way other than resizing the table. Maybe if the columns are defined with proportional widths, we could shrink the columns first to its minimum before growing the table.

[Optional] we can consider adding a feature to let darg rightmost column and get total width changes to the DataTable2

I think this could be a bit cumbersome, specially for large tables with many columns, you would have to scroll to the rightmost column in order to give more space for other columns. It would be much easier if the table grows/shrink with the column.

Here're some thoughts which I'm not quite sure yet in terms of implementability/consictency, your considerations are welcome:...

I agree with this, we could design a draggable widget that is both compatible with desktop and mobile, or give an option to select which mode you want to use (desktop / mobile) or put some parameters to customize the looks. I'll see what I can do.

One more thing, DataTable2 is stateless while PaginatedDataTable2 is statefull, apparently dragging columns will require to adress that issue cause it might be to reasonable to have that behavior implemented in both widget. Yet making DataTable2 stateful might be a poor option cause it is treated as direct replacement to Flutter's DataTable which is statless - not usre if switching DataTable2 to staefull might present any challenges in this regard

Yeah, this is a tricky subject, I didn't implement column resizing on DataTable2 to keep it stateless. Maybe we should clarify in the code comments that the function onColumnResized must be implemented to do the actual resizing. We could also create a new stateful widget, or just keep the implementation on PaginatedDataTable2.

Another thing I found, I tested this feature on another computer on Linux and I found the lag / misalignment problem a bit worse, I'll see if I can find a better approach for the resizing.

Santiago Conti and others added 4 commits May 29, 2022 19:48
- Added validation of minimum column size
- Added controller for data columns
- Now columns custom widths are kept when sorting columns or repainting the table
- Now non fixed width columns are resized first, if minimum is reached, the total table width is increased
- When a column is resized, is treated as fixed width
- During resizing, the column size now follows the mouse pointer
- Removed resizable column in DataTable2 example, as is not intended to be used here
- Added resizable columns to route options of PaginatedDataTable2 example
@nerdknight
Copy link
Author

I made some progress on the pending tasks. These are the major changes:

  • Now when resizing columns, the non fixed width columns are resized first before resizing the table
  • If a non fixed width column is resized, it's treated as fixed
  • Added customizable resize widget. It can be set to desktop or mobile mode, customize it's color and real time rendering

I'm wondering if it would be best to create a new stateful DataTable class to implement the resizing there and keep all the implementation in a data table. Let me know your thoughts.

@maxim-saplin
Copy link
Owner

maxim-saplin commented Jun 7, 2022

@nerdknight - thanks! The sample looks great, I like the way you styled the draggable. Here're the improvements to put into the backlog in regards to it's looks:

  • Find suitable styles in Material theme's and use those for colors, put a note in inline docs in regards to which styles are used to customize the looks
  • Consider shifting the draggable half it's width to the right to have it displayed exactly in between, or even not displayed (color/transparent - just like in Excel when you resize columns)

In terms on stateless/stateful I think there's no other better option rather than create a new widget StatefulDataTable2 and put the resizing logic there (encapsulating/aggregating DataTable2). There's also scrollController which can be moved there. In such a case PaginatedDataTable2 will encapsulate StatefulDataTable2 which will handle all the resizing, it can then simply forward constructor params related to column resizing and don't care about any of the resizing responsibilities. In this case I don't think columnDataController is needed - that can be an extra feature, expose it to the external client to let devs programatically change column sizes (I'd call it then columnSizeController), though wouldn't put much of an effort at this stage.

Speaking of DataTable2, it's necessary to make as few changes as possible to it's internals and move most of the logic to StatefulDataTable2 - keep the class Open for extensions but Closed for modifications - at least as much as possible. Besides DataTable2 and thus DataColumn2 should not leak any of the stateful features, such as resizing, there shouldn't be any public APIs in that regards - that's why I've introduced ResizableDataColumn2 inherited from DataColumn2.

I've committed some drafts of the above mentioned refactoring. Let me know what you think.

@maxim-saplin
Copy link
Owner

Speaking of column resizing, please see below an interesting bug. Please not that at the beginning of the video resizing columns leads to flexing all rows around. Yet when you change width of the Name column all other columns to the right get resized while column to the left are untouched - the second part IMO is the right behaviour:

resizing.bug.mov

Here's how I see resizing behaviour:

  • Resizing column leads to it's change of width via taking or giving space to whatever columns there're to the right, space to the left must be fixed/untouched
  • By default the total width of StatefulDataTable2 should remain unchanged, until StatefulDataTable2.lockTableWidth is set to false

Also a separate example (1st level route) is required for the resizing scenarious with 2 subroute - one depicting PaginatedDataTable2 and another StatefulDataTable2, that will make examples easer to understand

@nerdknight
Copy link
Author

The refactor you made looks good, I'll start working with that and the other proposed changes.

Speaking of column resizing, please see below an interesting bug. Please not that at the beginning of the video resizing columns leads to flexing all rows around. Yet when you change width of the Name column all other columns to the right get resized while column to the left are untouched

The actual behavior is a bit awkward, but it was intended this way: all the columns with proportional size (fixedWidth property set to null) to he left or right get resized, the columns defined with the fixedWidth property don't. When a proportional width column is resized, it's treated as fixed width, that's why on the second part of the video the columns to the left are no longer resized.

By default the total width of StatefulDataTable2 should remain unchanged, until StatefulDataTable2.lockTableWidth is set to false

It could be implemented, but with the actual behavior, if lockTableWidth is set to true and all the columns are set with fixedWidth, the resizing will not be possible. Maybe the fixed columns should be resized too. I'll see what I come up with.

@maxim-saplin
Copy link
Owner

maxim-saplin commented Jun 9, 2022

IMO, fixedWidth was not the best choice of naming, something like absoluteWidth would be more relevant. It's just an alternative way of defining default or standard sizing behavior for columns - you either define pixels and assume column doesn't respond to table width changes or provide relative size via S, M or L. None of the options must force the resizing logic to ignore any of the 2 kinds of rows. After all we have the resizable column type with a separate flag devoted to controlling locking or unlocking resizing, no need to add another parameter (checking if column is fixedWidth size). Though adding a documentation remark of stateful table regarding fixed column sizes might be useful.

Speaking of implementation detail, I think that looking into using fixed width columns to achieve column resizing upstream with zero to None changes to DataTable2 might be a good option. I.e in the StatefulDataTable2 on resizing thr build() method can instantiate a new table supplying it with clones of provided columns BUT having their fixedWidth set to specific values.

Note, a method on ColumnSizeControl controller can be added to reset column width adjustments.

- Implemented StatefulDataTable2
- Added new example for StatefulDataTable2
- Now PaginatedDataTable2 uses StatefulDataTable2 instead of DataTable2
@maxim-saplin
Copy link
Owner

maxim-saplin commented Jun 13, 2022

Checked the recent commit, assuming it is still a WiP I won't be treating it as the one requiring much of feedback. Though DataTable2.buildResizeColumnWidget - leaks the resizing capabilities built into StatefulDataTable2, that's smth that must be avoided for cleaner design.

I'm also curious if you have investigated rebuilding DataTable2 inside StatefulDataTable2 and recreating columns with fixed width upon resizing.

@maxim-saplin
Copy link
Owner

Thanks for tidying up the examples and presenting a new one!

@nerdknight
Copy link
Author

Thanks for your feedback maxim. I haven't had much time to work on this lately, an yes the refactor is still work in progress, if you like I could do partial commits so you can see the progress and give some feedback.
In order to avoid passing DataTable2.buildResizeColumnWidget in the constructor, I thought I may create another private class inheriting DataTable2 to build the widget there overriding DataTable2._buildHeadingCelland keep DataTable2 as clean as possible.

Something similar may be done with ColumnDataController, but I'll see it when I rework the resizing logic. I still may need to get the actual column sizes that are calculated in DataTable2._calculateDataColumnSizes though.

Another question I have, what do we do if we have a StatefulDataTable2 with a mix of fixed and non-fixed width columns and the window resizes? The actual behavior of DataTable2 is to resize only non-fixed-width columns. If we treat all columns as fixed, do we resize all columns proportional regardless of their initial settings? or resize only the rightmost column? and if some columns are not resizable? should they keep their initial settings?. I implemented the current logic to maintain this behavior of DataTable2. Rebuilding DataTable2 was one of the approaches I tried earlier but I think I discarded it for this reason.

@maxim-saplin
Copy link
Owner

Another question I have, what do we do if we have a StatefulDataTable2 with a mix of fixed and non-fixed width columns and the window resizes? The actual behavior of DataTable2 is to resize only non-fixed-width columns. If we treat all columns as fixed, do we resize all columns proportional regardless of their initial settings? or resize only the rightmost column? and if some columns are not resizable? should they keep their initial settings?. I implemented the current logic to maintain this behavior of DataTable2. Rebuilding DataTable2 was one of the approaches I tried earlier but I think I discarded it for this reason.

IMO, if a column is resized it keeps that size no matter if the width of parent is changed. Should the parent grow and all columns have their widths altered, they should stay that way. The only column changing it's width should be the rightmost one.

Optionally there can be a special mode which can adjust manually altered columns proportionally to parent change - though that mode is not trivial to describe (e.g. what if there's already a horizontal scroll bar and table total width is greater than viewport)

@maxim-saplin
Copy link
Owner

Hi @nerdknight , should you still be looking at the PR... I think I've come up with a more elegant solution to having state which allows to keep DataTable2 stalest and avoid creating StatefulDataTable2. You can use a stateful widget inside and builder pattern to feed column width's inside. You can check the recent commit and SyncedScrollControllers widget.

@maxim-saplin
Copy link
Owner

maxim-saplin commented Jul 20, 2022

Hey there. It is taking me time to review the changes. I'm going slowly as there's a lot of new code and not trivial one. Started with refactoring attempting to make the changes easier to understand.

I'm still not very happy with the behaviour of resizing of a column and having leftmost columns changing sizes as well, that seems a bit odd to me. Though I am still not well aquainted with the code to be specific of implementation suggestions.

@maxim-saplin
Copy link
Owner

I'm still not very happy with the behaviour of resizing of a column and having leftmost columns changing sizes as well, that seems a bit odd to me. Though I am still not well aquainted with the code to be specific of implementation suggestions.

Looks like you've changed that behaviour in the recent commits. Thanks!

@nerdknight
Copy link
Author

I'm still not very happy with the behaviour of resizing of a column and having leftmost columns changing sizes as well, that seems a bit odd to me. Though I am still not well aquainted with the code to be specific of implementation suggestions.

Looks like you've changed that behaviour in the recent commits. Thanks!

Hi Maxim, yes the previous behavior was a bit odd. Now columns to the left don't change size, the columns to the right resize if they are defined with proportional size. Let me know if you have any suggestions.

@stan-at-work
Copy link

@maxim-saplin Could you close this branch ?

@stan-at-work
Copy link

@maxim-saplin is it done ?

@maxim-saplin
Copy link
Owner

@maxim-saplin is it done ?

It is not

@stan-at-work
Copy link

@maxim-saplin

?

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.

4 participants