-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Qt: Implement GameCube memcard manager #6327
Conversation
spycrab
commented
Jan 25, 2018
•
edited
Loading
edited
3efb36a
to
5a22966
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.
For whatever reason GitHub won't load the .cpp file for GCMemcardManager (even in different browsers) for me, so I'll have to do this old-school bulleted-list style.
Line:
-
7:
<iostream>
should probably be removed. -
130:
if (m_slot_memcard[slot] != nullptr)
can be inverted to unindent the large body of code it contains. -
132, 218, 291, 313, 348, 362:
auto* memcard = m_slot_memcard[m_active_slot].get();
can likely just be:
auto& memcard = m_slot_memcard[m_active_slot];
since it's only ever used to call GCMemcard functions.
- 368:
std::vector<QPixmap> images;
images.resize(2);
can be:
std::vector<QPixmap> images(2);
- 383:
<algorithm>
should be included - 407:
image.fill(0xFFFFFFFF);
can likely be changed toimage.fill(Qt::white);
.
Other:
i < 2
seems to be quite frequent as the condition for loops, perhaps it would be better to use a constant likeNUM_SLOTS
in these cases. e.g.
class blah
{
public:
// ...
private:
// ...
// Slots
static constexpr int NUM_SLOTS = 2;
std::array<std::unique_ptr<GCMemcard>, NUM_SLOTS> m_slot_memcard;
std::array<QGroupBox*, NUM_SLOTS> m_slot_group;
std::array<QLineEdit*, NUM_SLOTS> m_slot_file_edit;
std::array<QPushButton*, NUM_SLOTS> m_slot_file_button;
std::array<QTableWidget*, NUM_SLOTS> m_slot_table;
std::array<QLabel*, NUM_SLOTS> m_slot_stat_label;
};
or
class blah
{
public:
// ...
private:
// ...
// Slots
struct Slot
{
std::unique_ptr<GCMemcard> memcard;
QGroupBox* group;
QLineEdit* file_edit;
QPushButton* file_button;
QTableWidget* table;
QLabel* stat_label;
};
static constexpr int NUM_SLOTS = 2;
std::array<Slot, NUM_SLOTS> m_slot;
};
then just use that.
#include <QDialog> | ||
|
||
#include "Common/MsgHandler.h" | ||
#include "Core/HW/GCMemcard/GCMemcard.h" |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
#pragma once | ||
|
||
#include <array> | ||
#include <memory> |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
||
#include <QDialog> | ||
|
||
#include "Common/MsgHandler.h" |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Everything is working fine here, but, I have a stupid request. Some of the saves have an animated icon. Can we animate those in the menu? I know that's a dumb request, but it'd look super cool and make the icon menu look better. Alternatively, only show the first image in an animation since it makes it look super awkward. |
I've run into some bugs as well. There's a lot of garbage text in some entries, such as my Mario Tennis one having wierd things. Will grab some screenshots on my other PC. |
c0ed2b6
to
7e5f41d
Compare
c5c741e
to
68dc639
Compare
68dc639
to
90699c2
Compare
90699c2
to
21db1e9
Compare
@JMC47 Good to go now? Or is there still something that bothers you? |
Everything is good here. I want to feature this in the progress report if it can be merged today. |