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

Introduce a new example using a QSortFilterProxyModel #174

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

Conversation

Ahuge
Copy link

@Ahuge Ahuge commented Feb 11, 2025

This implements a custom QAbstractItemModel and QSortFilterProxyModel

@mappu
Copy link
Owner

mappu commented Feb 11, 2025

Nice, thank you for the PR! It compiled fine for me and sorting the Size column does correctly understand the byte value of the files.

image

if role == qt.UserRole {
number, err := strconv.Atoi(cell.data[role])
if err != nil {
fmt.Printf("Error converting %s to int: %v\n", number, err)
Copy link
Owner

Choose a reason for hiding this comment

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

When starting up the example, I got these terminal messages:

Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax
Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax
Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax
Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax
Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax
Error converting %!s(int=0) to int: strconv.Atoi: parsing "": invalid syntax

They repeat when attempting to sort by the Name column,

Copy link
Author

Choose a reason for hiding this comment

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

Ah thanks I will fix that!

if !parent.IsValid() {
item = &rootItem
} else {
item = (*TreeItem)(parent.InternalPointer())
Copy link
Owner

Choose a reason for hiding this comment

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

There are some extra rules when passing pointers between Go and C. One of the rules is: If there is no Go pointer to the TreeItem, only a C++ pointer, then Go might GC the variable, and this could become a use-after-free.

Is there any other place this TreeItem is reachable other than from Qt (C++) memory? If not, a workaround is to either (A) ensure that the TreeItems are stored in some long-lived application struct or global variable; or (B) call runtime.Keepalive() at some later point.

Copy link
Owner

Choose a reason for hiding this comment

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

Or (C) Wrap the thing in a cgo.Handle before putting it in Qt space, and unwrap it when pulling it out again. This is what Miqt mostly does internally.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so the TreeItem is created in PopulateData and the rootItem keeps track of its children.
And we keep a reference to the rootItem.

I don't know if that is enough to keep it alive.

I am not super clear if pulling in a a pointer from the QModelIndex and then casting it to a TreeItem pointer is the right way to do that, I am open to another way to get the modelItem from a QModelIndex if there is one.

item = (*TreeItem)(parent.InternalPointer())
If I call runtime.KeepAlive(item) everytime that I get the InternalPointer, will that cause a memory leak?
Isn't QModelIndex.internalPointer() a go pointer anyways?

cellData = append(cellData, NewTreeCell(qt.DisplayRole, *qt.NewQVariant11("Big File")))
cellData = append(cellData, NewTreeCell(qt.DisplayRole, *qt.NewQVariant11("10.7 MiB")))
item := NewTreeItem(cellData, rootItem, false)
err := item.SetData(1, qt.NewQVariant11("11225400"), qt.UserRole)
Copy link
Owner

Choose a reason for hiding this comment

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

The sort size is stored in a QVariant as a string. Then later, we parse it. It might be simpler to use the QVariant constructor that stores an int directly,

Copy link
Author

Choose a reason for hiding this comment

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

Good call

treeView.SetModel(sortModel.QAbstractItemModel)

dialogLayout.AddWidget3(treeView.QWidget, 0, qt.AlignCenter)
return dialog
Copy link
Owner

Choose a reason for hiding this comment

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

In the default view, the Size column is initially truncated, so I went to resize the window larger. But something about the QSizePolicy is meaning the treeview will not grow when the window is resized (although it does shrink). I think this could be fixed in a 1 line change, but I haven't checked exactly.

image

image

Copy link
Author

Choose a reason for hiding this comment

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

So I took a deeper look at this.

I can't figure out how to make this work as I would expect.

The only way I can get it working is with the following hack:

dialog.OnResizeEvent(func(super func(event *qt.QResizeEvent), event *qt.QResizeEvent) {
		dialog.ResizeWithQSize(event.Size())
                // Width minus dialog.ContentMargins Left/Right
		treeView.Resize(event.Size().Width()-20, treeView.Size().Height())
	})

But having written Qt before, obviously that is a messy hack.

Is there a possibility that the default Resize events aren't being propagated down to child widgets?

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.

2 participants