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

feat: implement MacosTable #320

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

feat: implement MacosTable #320

wants to merge 7 commits into from

Conversation

pithuene
Copy link

@pithuene pithuene commented Nov 18, 2022

I implemented a MacosTable widget which looks similar to the macOS NSTableView (adresses #262 and #199).
The table features ordering by column and row selection.
I have also added some tests for the basic operations.
I am sure there is still a lot to improve, both in code and in UI, but I hope this can serve as a starting point.

This is a screenshot of the table widget running in a process monitor application I am building.
Note that this is running on linux and the font is not the one used on macOS.
Screenshot_2022-11-18_14-13-43

Pre-launch Checklist

  • I have run dartfmt on all changed files
  • I have incremented the package version as appropriate and updated CHANGELOG.md with my changes
  • I have added/updated relevant documentation
  • I have run "optimize/organize imports" on all changed files
  • I have addressed all analyzer warnings as best I could

@Yetispapa
Copy link

This is awesome! @GroovinChip, is there something in the way for merging?

@macosui macosui deleted a comment from stMerlHin Jan 13, 2023
Copy link
Collaborator

@GroovinChip GroovinChip left a comment

Choose a reason for hiding this comment

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

Hello @pithuene, and thank you for this PR! I'm excited to see a new widget being added, especially this one!

Please review the feedback I've left and implement the requested changes. Additionally, please update the CHANGELOG.md file.

Thanks!

lib/macos_ui.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_columns.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_row.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_row.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_selection.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_view.dart Outdated Show resolved Hide resolved
lib/src/table/macos_table_view.dart Outdated Show resolved Hide resolved
@GroovinChip GroovinChip linked an issue Jan 14, 2023 that may be closed by this pull request
@pithuene pithuene marked this pull request as draft January 16, 2023 17:27
GroovinChip and others added 2 commits January 16, 2023 18:33
date_picker_test.dart was failing due to not accounting for going from January to December and vice-versa.
@pithuene
Copy link
Author

Hey @GroovinChip, thanks a lot for giving me feedback on my pull request!
I tried to implement your requested changes but am not sure what you were asking for in the changelog file.

Thanks again for your time!

@pithuene pithuene marked this pull request as ready for review January 16, 2023 18:53
@pithuene pithuene marked this pull request as draft January 16, 2023 19:34
@pithuene pithuene marked this pull request as ready for review January 16, 2023 19:39
@GroovinChip
Copy link
Collaborator

Hey @GroovinChip, thanks a lot for giving me feedback on my pull request! I tried to implement your requested changes but am not sure what you were asking for in the changelog file.

Thanks again for your time!

I mean add an entry that describes what the new version adds or changes. If you look at the file you'll understand.

lib/src/layout/table/macos_table_columns.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_columns.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_datasource.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_selection.dart Outdated Show resolved Hide resolved
lib/src/layout/table/macos_table_view.dart Outdated Show resolved Hide resolved
@GroovinChip GroovinChip self-requested a review January 24, 2023 09:08
Copy link
Collaborator

@GroovinChip GroovinChip left a comment

Choose a reason for hiding this comment

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

One more thing - can you please post a screenshot of the table in dark mode? I want to make sure that it looks good with dark theme.

Also can you please run flutter format on macos_table_datasource.dart

@Yetispapa
Copy link

@pithuene looking forward for the merge! Can you maybe give me an example, how the ordering actually work?

It seems, when I change the datasource like so:


StreamBuilder(
          stream: widget.bloc.items,
          initialData: widget.bloc.items.value,
          builder: (BuildContext context, snapshot) {

....

    changeOrder: (order) {
      // this will update the StreamBuilder
      widget.bloc.changeOrder(order.direction.name);
      return true;
    },

The ordering gets messed up somehow. Can you help me here out. Thanks

@pithuene
Copy link
Author

I can take a look @Yetispapa but I would need more context, its hard to tell much just from the code you've shown.
You need to implement the ordering yourself (which you probably did in bloc.changeOrder, but I can't see).
Also I am not sure why you wrapped the table in a StreamBuilder, you should call dataChanged on the data source after the data has changed and let the table do the rebuilding internally.

@Yetispapa
Copy link

@pithuene, thanks for the quick repsonse. Alright. Let me check it again.

@GroovinChip
Copy link
Collaborator

@pithuene anything new to report on this?

@pithuene
Copy link
Author

pithuene commented Feb 4, 2023

@GroovinChip not yet. I've been too busy the last few weeks, might find the time next week though.

@GroovinChip
Copy link
Collaborator

No problem my dude, take your time. Your life comes first :)

@GroovinChip GroovinChip added the help wanted Extra attention is needed label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: NSTableView
3 participants