-
Notifications
You must be signed in to change notification settings - Fork 632
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
Conversation
@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? |
There was a problem hiding this 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.
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. |
To maybe clarify a little what I meant with putting import { ... } from "std-widgets.slint";
import { StandardListView } from "std-portable-widgets.slint"; Such that 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 Edit: Hmm, maybe it doesn't even need to be in a different file, but each 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 import { ListView, ListItem } from "std-widgets-impl.slint"; To avoid recursive importing. |
Ah yes. Sorry I was a little bit confused. I will give it a try. |
@bjorn thank you a lot for the hint. Sure that works. Is now implemented. All styles references now the same |
374475a
to
fdb0b96
Compare
There was a problem hiding this 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. :-)
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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.)
85ed843
to
9ac4b87
Compare
Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
Co-authored-by: Thorbjørn Lindeijer <bjorn@lindeijer.nl>
9ac4b87
to
1baf0b5
Compare
There was a problem hiding this 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
|
||
if (has_focus) { | ||
option.state |= QStyle::State_HasFocus; | ||
option.state |= QStyle::State_Selected; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.)
StandardListView
isn't visible #3548StandardListView
implementation for all styles