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

ENH: Provides control on item enability and selectability for DataView #1023

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xamcost
Copy link

@xamcost xamcost commented Sep 16, 2021

Hi there! This microscopic PR is intended to provide ways to control if cells in a table rendered with DataView can be selected or disabled., via new methods added to data_view.AbstractDataModel.

The primary need I'd like to answer with this PR is to offer the possibility to disable cells, or make them non-selectable, depending on some conditions in the underlying data. This is useful, for instance, to prevent interactions with some data items while they are used elsewhere. Feel free to close this PR if you have a better idea to tackle this!

Example

In pyface.examples.data_view.row_table_example, if you subclass the data model to implement is_enabled like this:

class ExampleDataModel(RowTableDataModel):

    def is_enabled(self, row, column):
        if len(row) == 0:
            return True

        obj = self.data[row[0]]
        return column_data.get_value(obj) > 30

and pass it to the data view widget, you should see this:

Screenshot 2021-09-17 at 00 20 01

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I don't think this is the right implementation of this, unfortunately.

We don't want the abstract data model growing additional features, particularly things which are directly related to display and interaction. This really belongs in the display layer, but we didn't get that implemented. At it's core you should be able to have two views of the same underlying data with different enabled/selected behaviour.

So in the current design, where it belongs is in the ValueType. I would not be opposed to having the value type grow attributes for being editable and selectable, with the understanding that that might change in the future. The behaviour and code should likely look an awful lot like the code for handling editability.

Also, needs tests!

@corranwebster
Copy link
Contributor

Secondary question - we already have a mechanism to make cells not editable: that presumably doesn't handle your immediate need?

@xamcost xamcost marked this pull request as draft September 17, 2021 09:20
@xamcost
Copy link
Author

xamcost commented Sep 17, 2021

Thanks for having a look!

This really belongs in the display layer, but we didn't get that implemented. At it's core you should be able to have two views of the same underlying data with different enabled/selected behaviour.

So in the current design, where it belongs is in the ValueType.

Sure, I hesitated to do put this in ValueType.. I did it in DataModel for a bad reason: it allows you to set the behaviour once for all indexes, while in case of a table with multiple ValueTypes, you would have to implement is_enabled for each of them if you want, for instance, to disable a full row or column. Reading you, I get that this should be the role of the display layer you mentioned, while DataModels would simply take care of shaping/accessing the data, is that right ?

Also, needs tests!

Ahah sure! I forgot to make this PR a draft, it is done now.

Secondary question - we already have a mechanism to make cells not editable: that presumably doesn't handle your immediate need?

For my use case, I want a visual feedback and having the item non selectable. The idea is to show to the user that the item is temporarily disabled, following an action she/he did on it that typically takes a few seconds.

@xamcost
Copy link
Author

xamcost commented Sep 17, 2021

I would not be opposed to having the value type grow attributes for being editable and selectable, with the understanding that that might change in the future. The behaviour and code should likely look an awful lot like the code for handling editability.

@corranwebster I am good with moving that code to ValueType, although if it is very likely to change or to conflict with existing aspirations in this repo, then I think it is better to close this PR: I can just implement a small patch where I need it. What do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants