-
-
Notifications
You must be signed in to change notification settings - Fork 833
rewrite Qt library frontend #2570
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
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
efe7d70
to
11ad94c
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.
A few minor notes so far, but mainly I think the dense code needs some better spacing to break up the logical blocks, and perhaps some comments explaining how the methods work. I'm pretty tired atm so my eyes kinda glossed over on some of the blocks of code.
One big thing is that now that SHA-1 stuff has been merged we'll probably want to figure out how to present that. Maybe remove the CRC32 column for now and figure out optional columns later, including CRC32 and SHA-1? Or leave it for a "more info" dialog when/if we get there.
COL_PLATFORM = 2, | ||
COL_SIZE = 3, | ||
COL_CRC32 = 4, | ||
MAX_COLUMN = 4, |
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.
Generally speaking, I prefer using an exclusive bound because you don't need to manually assign the value that way. You'll need to update the <= to < in places of course, but I find it somewhat easier to maintain. Especially if we're gonna add columns.
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 broadly agree but in terms of the implementation I have more reason to refer to the last column than to the number of columns; it's not just there for iteration. I'll make the switch if you insist but I'll be adding some - 1
's in places and removing a + 1
in another.
switch (platform) { | ||
#ifdef M_CORE_GBA | ||
case mPLATFORM_GBA: | ||
return QObject::tr("GBA"); |
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 get rid of the translation?
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.
Because these are used directly as hash keys, so translating them would break it.
318cb0b
to
86df254
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.
Can you fixup the git history so the fix commits are squashed? Keeping the backend and frontend commits separate is fine, I just don't want the whole history.
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.