-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
base: master
Are you sure you want to change the base?
Conversation
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.
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.
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.
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.
Unsure it's related, but I got this warning when testing: |
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. |
Rebased in order to pick up the platform icons. |
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. |
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.
Some small nits and a question
Oops. Forgot to add the new icons to the qrc. |
Can you rebase this on current master? |
a35d44f
to
8e484ac
Compare
964b303
to
93bf6d2
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.
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.
318ccb2
to
e4cc3c2
Compare
Please rebase |
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.
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.
b437064
to
f415084
Compare
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.