Add path for EEPROM #57
Conversation
settings.ui
Outdated
| @@ -205,6 +205,39 @@ | |||
| </layout> | |||
| </widget> | |||
| </item> | |||
| <item> | |||
| <widget class="QGroupBox" name="groupBox_11"> | |||
There was a problem hiding this comment.
Would be nice if we could name these widgets as well (even if it's just a context prefix). For example, this one could be eepromGroupBox.
There was a problem hiding this comment.
I agree, I'll fix it for the EEPROM settings here and maybe fix the others in another PR? There's a lot of poorly named objects in the project
There was a problem hiding this comment.
Agreed. Whether you updated them here or in a future pr, we should create an issue for this to help with visibility.
There was a problem hiding this comment.
I changed the EEPROM object names based on what we'd talk about on discord. I'll probably mention that in another issue/PR, assuming this naming convention is fine with others.
Libbers
force-pushed
the
eeprom-path
branch
2 times, most recently
from
276ddfe
to
86bd445
Compare
settings.ui
Outdated
| <widget class="QLineEdit" name="eepromPath"/> | ||
| </item> | ||
| <item row="0" column="2"> | ||
| <widget class="QPushButton" name="setEepromPath"> |
There was a problem hiding this comment.
This name is weird.
It implies it must be pressed in order for the eepom path become active.
Something like chooseEepromPath or browseEepromPath is probably more suitable.
There was a problem hiding this comment.
Agreed, however, I was just following the previous naming scheme of the other buttons, before I was made aware that there really isn't one =)
There was a problem hiding this comment.
Changed to browseEepromPath
settings.ui
Outdated
| </property> | ||
| <layout class="QGridLayout" name="eepromGridLayout"> | ||
| <item row="0" column="0"> | ||
| <widget class="QLabel" name="eepromImageLabel"> |
There was a problem hiding this comment.
nit: For consistency, this should be eepromPathLabel or the below names should be eepromImage or eepromImagePath
There was a problem hiding this comment.
Changed to eepromPathLabel
There was a problem hiding this comment.
I think it's fine to make an external EEPROM mandatory for users. I'd like to remove the EEPROM from XQEMU code in the future.
However, we must provide an EEPROM image before this is merged, so that existing users can still run XQEMU. I feel like we should rename https://github.com/xqemu/xqemu-hdd-image and bundle the EEPROM and the HDD together (alternatively, keep them separate and create xqemu-eeprom-image, then link it from our website).
Also, ideally we should refactor the settings handling first, or XQEMU-Manager will crash for people with an old config (on top of the lack of EEPROM).
I'm aware that this will delay this PR quite a bit (and potentially require some minor changes to this code), but I don't think that's an issue (as EEPROM isn't too useful until we support writeback).
I disagree, users are already required to provide kernel images, MCPX images and to actually use "the xbox" a valid HDD image. This seems very bikeshey on an already useful patch. |
|
Agreed. I'll probably fixup the remaining issues (if @kl0wn doesn't do it) and merge it soon. |
This PR adds a path to your EEPROM in settings. Currently this design forces a user to choose their own EEPROM and will not launch without it. It was discussed on discord that maybe XQEMU should ship with a dummy EEPROM in the future.
Below is a screenshot of the added option, please discuss if something does not look suitable.
This closes #43