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

rewrite Qt library frontend #2570

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

Conversation

ahigerd
Copy link
Contributor

@ahigerd ahigerd commented Jun 28, 2022

I've ripped out the existing implementation for the Qt library frontend and replaced it with a QAbstractItemModel. This should improve performance significantly. I've added some additional functionality along the way, like some reasonable behavior on window resizing and storing view configuration in the settings.

The grid view isn't implemented yet, obviously (but it wasn't available for selection in the settings, either), and I haven't removed LibraryGrid from the repository for future reference.

Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

Mostly just nits (though a lot of them), and a few bikeshedding questions. I trust your implementation skills for Qt models, so I didn't look over the implementation in too much depth.

src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.h Outdated Show resolved Hide resolved
Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

Some minor things since I've still glossed over the actual implementation fine details. I'll need to check how it works on my end before I can give approval anyway.

src/platform/qt/CMakeLists.txt Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.h Outdated Show resolved Hide resolved
src/platform/qt/test/library.cpp Outdated Show resolved Hide resolved
src/platform/qt/test/library.cpp Show resolved Hide resolved
@endrift
Copy link
Member

endrift commented Jul 2, 2022

Unsure it's related, but I got this warning when testing: QSortFilterProxyModel: index from wrong model passed to mapToSource

src/platform/qt/test/suite.h Outdated Show resolved Hide resolved
@ahigerd
Copy link
Contributor Author

ahigerd commented Jul 2, 2022

Unsure it's related, but I got this warning when testing: QSortFilterProxyModel: index from wrong model passed to mapToSource

It's related. Checking, I see it too. Haven't pinpointed it yet. I'll hook up QAbstractItemModelTester and see if it sheds light on it.

@ahigerd
Copy link
Contributor Author

ahigerd commented Jul 2, 2022

Rebased in order to pick up the platform icons.

@ahigerd
Copy link
Contributor Author

ahigerd commented Jul 2, 2022

Possible addition: https://github.com/ahigerd/mgba/pull/1/files

I don't know if this is a good idea or not, so I'm keeping it separate until it can be reviewed.

Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

Some small nits and a question

src/platform/qt/CMakeLists.txt Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryController.cpp Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
src/platform/qt/library/LibraryModel.cpp Outdated Show resolved Hide resolved
@ahigerd
Copy link
Contributor Author

ahigerd commented Jul 6, 2022

Oops. Forgot to add the new icons to the qrc.

@endrift endrift added this to the mGBA 0.11.0 milestone Jul 14, 2022
@endrift
Copy link
Member

endrift commented Oct 16, 2022

Can you rebase this on current master?

@ahigerd ahigerd force-pushed the alh/library branch 2 times, most recently from a35d44f to 8e484ac Compare November 2, 2022 00:39
@endrift endrift self-requested a review February 9, 2023 08:23
@ahigerd ahigerd force-pushed the alh/library branch 2 times, most recently from 964b303 to 93bf6d2 Compare February 16, 2023 04:25
Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

I don't have energy to review the rest of this atm (I should probably go to bed), but here's a review of the first 4 files for now, since there's stuff to change already. I'll try to get back to the rest soon.

src/core/library.c Show resolved Hide resolved
src/core/library.c Outdated Show resolved Hide resolved
src/platform/qt/CMakeLists.txt Outdated Show resolved Hide resolved
src/platform/qt/Window.cpp Outdated Show resolved Hide resolved
src/platform/qt/Window.cpp Outdated Show resolved Hide resolved
@ahigerd ahigerd force-pushed the alh/library branch 3 times, most recently from 318ccb2 to e4cc3c2 Compare April 28, 2023 00:39
@endrift
Copy link
Member

endrift commented May 9, 2023

Please rebase

Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

I don't have the mental energy to go over the Qt changes at the moment, but barring a style nitpick, the other stuff looks good so far. I'll take a closer look in a few days and then hopefully merge it.

src/platform/qt/utils.cpp Outdated Show resolved Hide resolved
@ahigerd ahigerd force-pushed the alh/library branch 2 times, most recently from b437064 to f415084 Compare November 4, 2024 01:58
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.

3 participants