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

added focus state to StandardLIstView #4086

Merged
merged 6 commits into from
Dec 8, 2023
Merged

Conversation

FloVanGH
Copy link
Member

@FloVanGH FloVanGH commented Dec 5, 2023

@FloVanGH FloVanGH requested review from tronical and ogoffart December 5, 2023 14:22
@FloVanGH
Copy link
Member Author

FloVanGH commented Dec 5, 2023

@ogoffart I also implemented the behavior for the qt style. It works but on macos the focus state is not displayed. Do you have an idea?

Copy link
Contributor

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Sorry for the duplicated code

It seems to me that maybe StandardListView should be implemented on a layer above the styles, so that one implementation suffices, which will derive from the ListView of the current style (and probably uses a display item from the style, too). Pretty much as if the StandardListView was part of the application Slint code. Is something like that possible?

Btw, personally I would expect StandardListView to be a thin convenience wrapper on top of ListView. It's weird to me to see stuff like "current item" and focus handling to be done only for the StandardListView, where I expect that due to the limited customization options provided by StandardListView, most people will need to rely on ListView instead and then will need to re-implement all that stuff too.

CHANGELOG.md Outdated Show resolved Hide resolved
@FloVanGH
Copy link
Member Author

FloVanGH commented Dec 6, 2023

Sorry for the duplicated code

It seems to me that maybe StandardListView should be implemented on a layer above the styles, so that one implementation suffices, which will derive from the ListView of the current style (and probably uses a display item from the style, too). Pretty much as if the StandardListView was part of the application Slint code. Is something like that possible?

Btw, personally I would expect StandardListView to be a thin convenience wrapper on top of ListView. It's weird to me to see stuff like "current item" and focus handling to be done only for the StandardListView, where I expect that due to the limited customization options provided by StandardListView, most people will need to rely on ListView instead and then will need to re-implement all that stuff too.

I also thought about it. I would love also to have a generic variant of the StandardListView that give you the possibility to define the fields and also the look of the items. But I think at the moment it is difficult to split the StandardListView from the style. The problem is that in the ListView itself is the magic for the virtualization. For the scrollbars it needs to inherit the ScrollViewer what is style dependent and so you have also the style dependency from the StandardListView. I. think we need to find a way for the future to better decouple that stuff.

I think also something like delegates in qml would also be a plus.

@bjorn
Copy link
Contributor

bjorn commented Dec 6, 2023

To maybe clarify a little what I meant with putting StandardListView in another layer, maybe it could work by putting it in a different import, for example:

import { ... } from "std-widgets.slint";
import { StandardListView } from "std-portable-widgets.slint";

Such that std-portable-widgets.slint could be from a different path, independent of the current style, and from that file one could implement it as follows:

import { ListView, ListItem } from "std-widgets.slint";

component StandardListView inherits ListView {
    in property <[StandardListViewListItem]> items;

    // ...

    for item[index] in items: ListItem {
        text: item.text;
        selected: index == root.current-item;

        // ...
    }
}

So that it will pick up the needed customization from whatever is the current style as usual.

At the same time, some of the focus and current item handling code could be moved into ListView, since it would be useful for any list, not just a "standard list".

Edit: Hmm, maybe it doesn't even need to be in a different file, but each std-widgets-base.slint could just have:

import { StandardListView } from "../common/standardlistview.slint";
export { StandardListView }

I see this is already a recurring pattern. Is there a reason it couldn't work for StandardListView? Its import may just need to be changed to:

import { ListView, ListItem } from "std-widgets-impl.slint";

To avoid recursive importing.

@FloVanGH
Copy link
Member Author

FloVanGH commented Dec 7, 2023

I see this is already a recurring pattern. Is there a reason it couldn't work for StandardListView? Its import may just need to be changed to:

Ah yes. Sorry I was a little bit confused. I will give it a try.

@FloVanGH
Copy link
Member Author

FloVanGH commented Dec 7, 2023

@bjorn thank you a lot for the hint. Sure that works. Is now implemented. All styles references now the same StandardListView implementation.

@FloVanGH FloVanGH force-pushed the florian/list-view-focus branch from 374475a to fdb0b96 Compare December 7, 2023 10:24
Copy link
Contributor

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

That looks much better indeed! I've left some additional thoughts and suggestions. :-)

internal/compiler/widgets/material-base/combobox.slint Outdated Show resolved Hide resolved
has-hover: i-touch-area.has-hover;
pressed: i-touch-area.pressed;
pressed-x: i-touch-area.pressed-x;
pressed-y: i-touch-area.pressed-y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the touch area not part of the ListItem? It seems things could be simplified a little if it was included, which it was for all styles before, except for the Qt style (thought the Qt style could be adjusted so it is also included there).

Otherwise, in all places where we're using ListItem (here and in the various ComboBox implementations), we need to instantiate the TouchArea and bind all these properties.

(but maybe you already considered the options... by including touch area in list item, one of course has to do those bindings in the list item for each style, but especially due to the combo box, it seems we need to do that anyway)

Copy link
Member

Choose a reason for hiding this comment

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

(Yes, the Qt style could be changed if it makes code sharing easier.)

@FloVanGH FloVanGH force-pushed the florian/list-view-focus branch from 85ed843 to 9ac4b87 Compare December 7, 2023 13:22
@FloVanGH FloVanGH force-pushed the florian/list-view-focus branch from 9ac4b87 to 1baf0b5 Compare December 7, 2023 17:12
Copy link
Member

@ogoffart ogoffart 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 an improvement

CHANGELOG.md Outdated Show resolved Hide resolved

if (has_focus) {
option.state |= QStyle::State_HasFocus;
option.state |= QStyle::State_Selected;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you added the selected state there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I tried to get the focus visible. But this also dos not helped.

has-hover: i-touch-area.has-hover;
pressed: i-touch-area.pressed;
pressed-x: i-touch-area.pressed-x;
pressed-y: i-touch-area.pressed-y;
Copy link
Member

Choose a reason for hiding this comment

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

(Yes, the Qt style could be changed if it makes code sharing easier.)

@FloVanGH FloVanGH merged commit 6d6b183 into master Dec 8, 2023
30 checks passed
@FloVanGH FloVanGH deleted the florian/list-view-focus branch December 8, 2023 09:59
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.

Focus on StandardListView isn't visible
3 participants