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

library: Add ListModel Entry #361

Merged
merged 20 commits into from Jul 4, 2023
Merged

library: Add ListModel Entry #361

merged 20 commits into from Jul 4, 2023

Conversation

SoNiC-HeRE
Copy link
Contributor

Closes #294

@SoNiC-HeRE SoNiC-HeRE marked this pull request as draft June 23, 2023 19:56
@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 23, 2023

What I'm trying to achieve is when i click on add items button it should add rows with labels to the existing listbox widget

Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Does that make more sense?

src/Library/demos/List Model/main.js Outdated Show resolved Hide resolved
src/Library/demos/List Model/main.js Outdated Show resolved Hide resolved
src/Library/demos/List Model/main.js Outdated Show resolved Hide resolved
@SoNiC-HeRE
Copy link
Contributor Author

@andyholmes would it be good to demonstrate search function as well?
I was thinking of adding a search entry to search for an item and then delete it; wdyt?

P.S:

  • Implemented Add and Remove functions for respective model
  • A little bit of UI improvement is needed

@andyholmes
Copy link
Contributor

There's no Gio.ListModel in your demo, that I can see.

The main principle to demonstrate here is to have a single Gio.ListModel instance, like Gtk.StringList or Gio.ListStore, and use it to automatically populate different widgets using Gtk.ListBox.bind_model() and Gtk.FlowBox.bind_model().

Your add/remove buttons should call Gtk.StringList.append() or Gio.ListStore.append() and the Gtk.ListBoxCreateWidgetFunc and Gtk.FlowBoxCreateWidgetFunc should create the appropriate widget for the container (e.g. Adw.ActionRow for Gtk.ListBox, Gtk.Box for Gtk.FlowBox).

Then if you want, you implement searching and filtering for either container by wrapping the Gio.ListModel in Gtk.FilterListModel and binding the model to that instead.

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 27, 2023

I'll make the required changes

@SoNiC-HeRE
Copy link
Contributor Author

Implemented for the following changes:

  • Used StringList to populate both List and Flow Box
  • Used ListModel methods for adding/removing
  • Used toggle mode to make the UI minimalistic and depict one usecase at a time

@SoNiC-HeRE SoNiC-HeRE marked this pull request as ready for review June 28, 2023 17:11
Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Much better, I think you're getting how model-view-controller works in GTK now!

With regard to your idea about a text entry, let's clean the UI up to use the Gtk.Stack as in the review suggestions, then how about adding a third page like Edit or something?

A third page could be a list box, flow box, or we could demonstrate the items-changed callback if you're feeling adventurous. Either way, for the third view we should put the Gtk.StringList inside another model like Gtk.FilterListModel, and have the entry change the filter.

Comment on lines 11 to 64
Box {
orientation: horizontal;
halign: center;
margin-bottom: 24;
styles ["linked"]

ToggleButton toggle_list_box {
active: true;
label: _("ListBox");
}

ToggleButton toggle_flow_box {
active: false;
label: _("FlowBox");
group: toggle_list_box;
}
}

Revealer reveal_list_box {
transition-duration: 300;
transition-type: slide_up;
margin-bottom: 24;
reveal-child: true;

Box {
halign: center;

ListBox list_box {
activate-on-single-click: true;
selection-mode: none;
width-request: 360;

styles ["boxed-list"]
}
}
}

Revealer reveal_flow_box {
transition-duration: 300;
transition-type: slide_up;
margin-bottom: 24;

Box {
halign: center;

FlowBox flow_box {
width-request: 360;
activate-on-single-click: true;
orientation: horizontal;
selection-mode: none;
styles ["card"]
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's clean this all up with some higher-level widget.

Suggested change
Box {
orientation: horizontal;
halign: center;
margin-bottom: 24;
styles ["linked"]
ToggleButton toggle_list_box {
active: true;
label: _("ListBox");
}
ToggleButton toggle_flow_box {
active: false;
label: _("FlowBox");
group: toggle_list_box;
}
}
Revealer reveal_list_box {
transition-duration: 300;
transition-type: slide_up;
margin-bottom: 24;
reveal-child: true;
Box {
halign: center;
ListBox list_box {
activate-on-single-click: true;
selection-mode: none;
width-request: 360;
styles ["boxed-list"]
}
}
}
Revealer reveal_flow_box {
transition-duration: 300;
transition-type: slide_up;
margin-bottom: 24;
Box {
halign: center;
FlowBox flow_box {
width-request: 360;
activate-on-single-click: true;
orientation: horizontal;
selection-mode: none;
styles ["card"]
}
}
}
Gtk.StackSwitcher {
stack: stack;
halign: center;
}
Gtk.Stack stack {
transition-duration: 300;
transition-type: crossfade;
vexpand: true;
Gtk.StackPage {
name: "listbox";
title: _("List Box");
child:
Box {
halign: center;
ListBox list_box {
activate-on-single-click: true;
selection-mode: none;
width-request: 360;
styles ["boxed-list"]
}
};
}
Gtk.StackPage {
name: "flowbox";
title: _("Flow Box");
child:
Box {
halign: center;
FlowBox flow_box {
width-request: 360;
activate-on-single-click: true;
orientation: horizontal;
selection-mode: none;
styles ["card"]
}
};
}
}

const add = workbench.builder.get_object("add");
const remove = workbench.builder.get_object("remove");

//ListModel initialization and binding
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//ListModel initialization and binding
// Model

Comment on lines 36 to 64
//Handling for ListBox
const isListBoxActive = toggle_list_box.get_active();
reveal_list_box.reveal_child = isListBoxActive;
if (isListBoxActive) {
add.connect("clicked", () => {
const newItem = `Item ${item}`;
model.append(newItem); // Append the new item as an array to the model
item++;
});
remove.connect("clicked", () => {
const length = model.get_n_items();
model.remove(length - 1);
});
}

//Handling for FlowBox
const isFlowBoxActive = toggle_flow_box.get_active();
reveal_flow_box.reveal_child = isFlowBoxActive;
if (isFlowBoxActive) {
add.connect("clicked", () => {
const newItem = `Item ${item}`;
model.append(newItem); // Append the new item as an array to the model
item++;
});
remove.connect("clicked", () => {
const length = model.get_n_items();
model.remove(length - 1);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we only need one set of these, since we're "controlling" the "model" in a model-view-controller pattern. Stripping this part down makes that a little bit clearer.

Suggested change
//Handling for ListBox
const isListBoxActive = toggle_list_box.get_active();
reveal_list_box.reveal_child = isListBoxActive;
if (isListBoxActive) {
add.connect("clicked", () => {
const newItem = `Item ${item}`;
model.append(newItem); // Append the new item as an array to the model
item++;
});
remove.connect("clicked", () => {
const length = model.get_n_items();
model.remove(length - 1);
});
}
//Handling for FlowBox
const isFlowBoxActive = toggle_flow_box.get_active();
reveal_flow_box.reveal_child = isFlowBoxActive;
if (isFlowBoxActive) {
add.connect("clicked", () => {
const newItem = `Item ${item}`;
model.append(newItem); // Append the new item as an array to the model
item++;
});
remove.connect("clicked", () => {
const length = model.get_n_items();
model.remove(length - 1);
});
}
// Controller
add.connect("clicked", () => {
const new_item = `Item ${item}`;
model.append(new_item);
item++;
});
remove.connect("clicked", () => {
const n_items = model.get_n_items();
model.remove(n_items - 1);
});

Comment on lines 66 to 86
// Check if ListBox is Active
toggle_list_box.connect("toggled", () => {
const isActive = toggle_list_box.get_active();
reveal_list_box.reveal_child = isActive;
if (isActive) {
console.log("ListBox toggled on");
} else {
console.log("ListBox toggled off");
}
});

// Check if FlowBox is Active
toggle_flow_box.connect("toggled", () => {
const isActive = toggle_flow_box.get_active();
reveal_flow_box.reveal_child = isActive;
if (isActive) {
console.log("FlowBox toggled on");
} else {
console.log("FlowBox toggled off");
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good idea to identify when the view has changed, but let's keep it simple so it doesn't distract from the rest of the demo.

Suggested change
// Check if ListBox is Active
toggle_list_box.connect("toggled", () => {
const isActive = toggle_list_box.get_active();
reveal_list_box.reveal_child = isActive;
if (isActive) {
console.log("ListBox toggled on");
} else {
console.log("ListBox toggled off");
}
});
// Check if FlowBox is Active
toggle_flow_box.connect("toggled", () => {
const isActive = toggle_flow_box.get_active();
reveal_flow_box.reveal_child = isActive;
if (isActive) {
console.log("FlowBox toggled on");
} else {
console.log("FlowBox toggled off");
}
});
// View
stack.connect("notify::visible-child", () => {
if (stack.visible_child === list_box) {
console.log("View: List Box");
} else {
console.log("View: Flow Box");
}
});

src/Library/demos/List Model/main.js Show resolved Hide resolved
Comment on lines 6 to 9
const toggle_list_box = workbench.builder.get_object("toggle_list_box");
const toggle_flow_box = workbench.builder.get_object("toggle_flow_box");
const reveal_list_box = workbench.builder.get_object("reveal_list_box");
const reveal_flow_box = workbench.builder.get_object("reveal_flow_box");
Copy link
Contributor

Choose a reason for hiding this comment

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

Much simpler 🙂

Suggested change
const toggle_list_box = workbench.builder.get_object("toggle_list_box");
const toggle_flow_box = workbench.builder.get_object("toggle_flow_box");
const reveal_list_box = workbench.builder.get_object("reveal_list_box");
const reveal_flow_box = workbench.builder.get_object("reveal_flow_box");
const stack = workbench.builder.get_object("stack");

Comment on lines 9 to 10
orientation: vertical;

Copy link
Contributor

Choose a reason for hiding this comment

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

We can let the containing box do the child spacing.

Suggested change
orientation: vertical;
orientation: vertical;
spacing: 24;

@SoNiC-HeRE
Copy link
Contributor Author

Okay, What I'm trying to do here is when the user clicks on the search button i want to check if the searched text matches one of the strings in model by updating the Filter.search property and then using Filter.match(searched_text) to evaluate whether it was found or not but i get encountered by an error which states searched_text is not of type GObject which is true; but I'm not really sure how to use the string for evaluation in filter;Also i created a new Gtk.PropertyExpression as:

 const filter = new Gtk.StringFilter({
 expression: Gtk.PropertyExpression.new(model, null, "strings"),
 ignore_case: true,
 match_mode: Gtk.StringFilterMatchMode.EXACT,
});

but got an error

gtk_string_filter_set_expression: assertion 'expression == NULL || gtk_expression_get_value_type (expression) == G_TYPE_STRING' failed

@andyholmes
Copy link
Contributor

So I think you're pretty close.

The filter is going to be passed an item from the GListModel, which is a GtkStringList, so it should be expecting a GtkStringObject and we want to get the GtkStringObject:string property:

const search_expression = Gtk.PropertyExpression.new(Gtk.StringObject, null, "string");

GtkStringFilter will use the expression to retrieve the item's string value, then compare it to another string (i.e. the search terms) by calling Gtk.Filter.match(), which always expects a GObject. Since we want to be able to filter, not just find, the match-mode should be set to Gtk.StringFilterMatchMode.SUBSTRING instead of EXACT:

const filter = new Gtk.StringFilter({
 expression: search_expression,
 ignore_case: true,
 match_mode: Gtk.StringFilterMatchMode.SUBSTRING,
});

When we get to the search entry, let's skip the button and just filter directly:

const search_entry = new Gtk.SearchEntry();

search_entry.connect("search-changed", (editable) => {
  console.log("Search terms changed");

  search_filter.search = editable.get_text();

  // This will not work, because you are passing a string
  // where a GObject is expected. Calling this function is
  // also what the filter does for you, so there's no need
  search_filter.match(editable.get_text());
});

Once that's done, I think we've done enough for the demo and we'll just cleanup the UX a bit. Namely I don't think any widgets should be underneath the view stack, because everything jumps around when items are added/removed/filtered.

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 30, 2023

@andyholmes I've applied the suggested changes; talking about UX - i was thinking to maybe change the widget that populates all the 3 models? Also we need to take care of the void space created cause of the SearchEntry widget, wdyt?

P.S: I've also kept the comment about filter.match() property in the code - thought it would be useful, i can remove it if you say

Comment on lines 84 to 88

/* This will not work, because you are passing a string
where a GObject is expected. Calling this function is
also what the filter does for you, so there's no need
search_filter.match(editable.get_text()); */
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this.

@andyholmes
Copy link
Contributor

@andyholmes I've applied the suggested changes; talking about UX - i was thinking to maybe change the widget that populates all the 3 models?

How do you mean change the widget? Do you mean have a separate GListModel in addition to the GtkStringList and swap them in and out?

Also we need to take care of the void space created cause of the SearchEntry widget, wdyt?

I think the reasonable place to have the entry is above the views, because they are what will change size the most. Either a margin, or just allowing the widget to shift when the stack page changes I think is okay.

P.S: I've also kept the comment about filter.match() property in the code - thought it would be useful, i can remove it if you say

Yeah, I think remove it. We're just demonstrating how to use GListModel here and how they can be wrapped in other models. We can cover actual use of filter models and sorting models in another demo.

@SoNiC-HeRE
Copy link
Contributor Author

@andyholmes I've applied the suggested changes; talking about UX - i was thinking to maybe change the widget that populates all the 3 models?

How do you mean change the widget? Do you mean have a separate GListModel in addition to the GtkStringList and swap them in and out?

Also we need to take care of the void space created cause of the SearchEntry widget, wdyt?

I think the reasonable place to have the entry is above the views, because they are what will change size the most. Either a margin, or just allowing the widget to shift when the stack page changes I think is okay.

P.S: I've also kept the comment about filter.match() property in the code - thought it would be useful, i can remove it if you say

Yeah, I think remove it. We're just demonstrating how to use GListModel here and how they can be wrapped in other models. We can cover actual use of filter models and sorting models in another demo.

I meant regarding this #361 (comment)

Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

A few more notes, and thoughts I had about UX.

ListBox list_box_editable {
width-request: 240;
activate-on-single-click: true;
selection-mode: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's set this to single and mark the remove button active when there is a selected row, then remove the item from the model with at the row's index position. I think that's sensible UX.

Comment on lines 76 to 87
Box {
styles ["linked"]
halign: center;
margin-bottom: 18;

Button add {
label: _("Add Items");
}
Button remove {
label: _("Remove Items");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this above the "Edit" page's list, and connect the whole thing so it looks something like this:

Box {
  orientation: vertical;
  spacing: 18;

  Box {
    styles ["linked"]

    SearchEntry search_entry {
      hexpand: true;
      placeholder-text: _("Start searching");
    }
    Button add {
      icon-name: "list-add-symbolic";
      tooltip-text: _("Add Item");
    }
    Button remove {
      icon-name: "list-remove-symbolic";
      tooltip-text: _("Remove Item");
    }
  }

  ListBox list_box_editable {
    activate-on-single-click: true;
    selection-mode: single;
    styles ["boxed-list"]
  }
}

}

Gtk.Stack stack {
transition-duration: 300;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
transition-duration: 300;

Not needed.


Gtk.StackSwitcher {
stack: stack;
halign: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

This one can stay, so the switch is nicely centered about the lists.

src/Library/demos/List Model/main.js Show resolved Hide resolved
Comment on lines 62 to 69

model.connect("items-changed", (list, position, removed, added) => {
console.log(
`position: ${position}, Item removed? ${Boolean(
removed,
)}, Item added? ${Boolean(added)}`,
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
model.connect("items-changed", (list, position, removed, added) => {
console.log(
`position: ${position}, Item removed? ${Boolean(
removed,
)}, Item added? ${Boolean(added)}`,
);
});

src/Library/demos/List Model/main.js Show resolved Hide resolved
@SoNiC-HeRE
Copy link
Contributor Author

Implemented the suggested changes:

  • Improved UI
  • Improved UX
  • Refined Code

Copy link
Contributor

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Looking really good.

There are some final UI fixes, then this one is good to go. I think this will be a good template for the GtkListView, GtkGridView and other GListModel demos.

src/Library/demos/List Model/main.js Show resolved Hide resolved
src/Library/demos/List Model/main.blp Show resolved Hide resolved
width-request: 360;
orientation: horizontal;
selection-mode: none;
styles ["card"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
styles ["card"]

title: "List Model";
description: _("List models are a simple interface for ordered lists of GObject instances");

Box {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so let's do this:

Except for the StackSwitcher and FlowBox items, go through and remove all the halign: center and width-request properties. Then put this Box inside an Adw.Clamp with maximum-size: 640; so you have something like:

Adw.Clamp {
  maximum-size: 640;
  child:
    Box current_box {
      // ...
    };
}

Then set hexpand: true and valign: start on list_box, flow_box and list_box_editable. That should even up the whole demo.

styles ["linked"]

SearchEntry search_entry {
hexpand: true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hexpand: true;

});

list_box_editable.connect("row-selected", () => {
remove.sensitive = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but it's possible to "unselect" all rows, so better to do something like:

Suggested change
remove.sensitive = true;
remove.sensitive = list_box_editable.get_selected_row() !== null;

const search_entry = workbench.builder.get_object("search_entry");

//Model
const model = new Gtk.StringList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const model = new Gtk.StringList();
const model = new Gtk.StringList({
strings: [
"Default Item 1",
"Default Item 2",
"Default Item 3",
],
});

I think this is the easiest way to avoid dealing with empty states in the demo. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's obviously better than a void space

Comment on lines 39 to 41
Box {
halign: center;
hexpand: true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this Box, the Frame is taking its place.

Comment on lines 89 to 92
const items = model.get_n_items();
if (items === 0) {
remove.sensitive = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be handled by the row-selected signal, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@SoNiC-HeRE
Copy link
Contributor Author

  • Implemented the suggested changes
  • Increased the size of flowbox items that visually looks better and occupies the available space more accurately
  • Moved the linkbutton to top since it did'nt look good below, lmk if i should place it to the original place

Copy link
Contributor

@andyholmes andyholmes 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 a really nice demo, well done 👍

I think we should definitely use this as a template for the list/grid/column view demos.

@andyholmes andyholmes merged commit d1ba596 into main Jul 4, 2023
@andyholmes
Copy link
Contributor

* Moved the `linkbutton` to top since it did'nt look good below, lmk if i should place it to the original place

I think that was a good choice. We might want to update and expand the links later, but I think we should get into the other list model widgets first, and see how we want to organize them.

sonnyp pushed a commit that referenced this pull request Jul 6, 2023
* Adds ListModel Demo

* Updated Code

* Used List Model

* Made Changes as Discussed

* Updated Code as per review

* Added new page and items-changed signal

* Implemented Filter-Model

* Minor Changes

* Minor Change

* Applied suggested Changes

* Refined Code

* Updated UX

* More Changes

* Updated Logic

* Finishing Up

* Updated UI

* Minor Tweak

* Enhanced UI and functionality

* Perfect Squares

* Minor tweaks and indentation fix
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Aug 13, 2023
* Adds ListModel Demo

* Updated Code

* Used List Model

* Made Changes as Discussed

* Updated Code as per review

* Added new page and items-changed signal

* Implemented Filter-Model

* Minor Changes

* Minor Change

* Applied suggested Changes

* Refined Code

* Updated UX

* More Changes

* Updated Logic

* Finishing Up

* Updated UI

* Minor Tweak

* Enhanced UI and functionality

* Perfect Squares

* Minor tweaks and indentation fix
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.

Add tutorial-demo for GListModel with GTK & Adwaita
2 participants