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

Game library #37

Closed
wants to merge 1 commit into from
Closed

Game library #37

wants to merge 1 commit into from

Conversation

BigBrainAFK
Copy link
Contributor

Adds game library like feature to the GUI.
File format detection per title based on magic numbers instead of file extension.
Caches file format results.

Adds game library like feature to the GUI.
File format detection per title based on magic numbers instead of file extension.
Caches file format results.
main.py Show resolved Hide resolved
class DiscImageFormat(object):
XISO = 'XISO'
ISO9660 = 'ISO9660'
Unknown = 'Unknown format'
Copy link
Member

@JayFoxRox JayFoxRox Jan 13, 2019

Choose a reason for hiding this comment

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

This should only act as an enum (ideally not used for display), so a number or unique identifer is better.
"Unknown format" is neither of these.

main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
main.py Show resolved Hide resolved
def setDirectory(self, obj):
options = QFileDialog.Options()
directory = QFileDialog.getExistingDirectory(self,
"Select DVD Directory",
Copy link
Member

Choose a reason for hiding this comment

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

nit: not only "DVD", but "Optical Disc Image", poor wording though. I'd probably pick "CD / DVD Image" or something.

@@ -206,7 +286,7 @@ def genArg(settings, name, port):
return args

@staticmethod
def generateLaunchCmd(settings, skipPathChecks=False):
def generateLaunchCmd(settings, main, skipPathChecks=False):
Copy link
Member

@JayFoxRox JayFoxRox Jan 13, 2019

Choose a reason for hiding this comment

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

This is ugly. Why does it need full access to the main object? Isn't it enough to pass the path to the DVD image?

Generally, I believe we'll probably (eventually) have some "virtual-xbox" state object, which basically says: "boot a virtual-xbox, which was left in this condition: revision 1.0, 128MB, bios flashed with XYZ, Halo disc in DVD tray, ..." (so a combination of settings and - possibly - user input).

@@ -274,8 +355,8 @@ def launchCmdToString(cmd):

return ' '.join(cmd_escaped)

def start(self, settings):
cmd = self.generateLaunchCmd(settings)
def start(self, settings, main):
Copy link
Member

Choose a reason for hiding this comment

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

Same as for generateLaunchCmd. Information hiding etc.

@@ -344,9 +425,12 @@ def __init__(self, *args):
self.inst = Xqemu()
self.settings = SettingsManager()
self.settings.load()
self.runButton.setText('Start')
self.pauseButton.setText('Pause')
self.screenshotButton.setText('Screenshot')
Copy link
Member

Choose a reason for hiding this comment

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

Where did this (or equivalent) move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not necessary as far as i can tell since the text will be set after launch due to the .ui files anyway

Copy link
Member

Choose a reason for hiding this comment

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

@mborgerson why was this done? Localization or something? Or just accidentally?

main.py Show resolved Hide resolved
self.gamesCache = []

# Disable resizing because it doesnt really do anything
self.setFixedSize(540, 293)
Copy link
Member

Choose a reason for hiding this comment

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

Disabling resizing isn't an option in my opinion.
This looks like a cruel hack, rather than good UI design.

for title in files:
filepath = self.settings.settings['dvd_folder_path'] + '/' + title
cached = self.libraryCache.checkExisting(filepath)
# This should fetch pmuch all infos from cache if possible
Copy link
Member

Choose a reason for hiding this comment

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

"pmuch"


SETTINGS_FILE = './settings.json'
LIBRARY_CACHE = './library.cache'
Copy link
Member

Choose a reason for hiding this comment

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

Regardless of the file format we eventually choose, give this a proper extension (.csv / .json).

@JayFoxRox
Copy link
Member

Rejected due to lack of progress / incompleteness.

We kept going back and forth about this on Discord when this PR was fresh, because there's some design limitations for LLE.
This PR still contains a lot of useful stuff (like disc type checks, before handing it to QEMU), so I hope someone will continue the work that @BigBrainAFK has done here.

As for the major GUI changes / game-selection, we suggest to consult XQEMU maintainers.

Closed.

@JayFoxRox JayFoxRox closed this May 15, 2019
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.

None yet

3 participants