GUI, BACKENDS: Add DLC Downloader #5134

ankushdutt · 2023-07-03T11:10:54Z

This PR will add support to allow downloading freeware games from the ScummVM GUI.

backends/dlc/dlcmanager.h

mgerhardy · 2023-07-04T20:04:41Z

maybe the api for mod.io can help to flesh out other store needs: https://github.com/modio/modio-sdk/tree/main

ankushdutt · 2023-07-05T07:12:23Z

maybe the api for mod.io can help to flesh out other store needs: https://github.com/modio/modio-sdk/tree/main

Thanks, I will take a look.

backends/dlc/dlcmanager.h

gui/download-games-dialog.h

backends/dlc/dlcmanager.h

sev-

I reviewed it, and there are a few relatively minor notes.

Also, I think you added way too many #ifdefs for the feature, e.g. we will not compile in scummvmdlc, only for debugging purposes, but the interface buttons and dialogs will not be compiled in this case.

backends/dlc/dlcdesc.h

sev- · 2023-08-06T12:55:28Z

backends/dlc/dlcdesc.h

+	Common::String extra;
+	Common::String engineid;
+	Common::String guioptions;
+	uint32 size;


Please initialize it, e.g. uint32 size = 0;. And the other two variables below as well.

sev- · 2023-08-06T12:59:10Z

backends/dlc/dlcmanager.h

+#include "common/singleton.h"
+
+#include "gui/object.h"
+#include "gui/launcher.h"


Please avoid these excessive includes in a header file. Use forward declaration, e.g.

namespace GUI { class LauncherDialog; } namespace DLC { class Store; ....

will be enough since you only refer to those as pointers in the header.

Leave only absolutely necessary includes here.

Done: d701264

sev- · 2023-08-06T19:39:18Z

backends/dlc/scummvmcloud.cpp

+		// extract the downloaded zip
+		Common::String gameDir = Common::punycode_encodefilename(DLCMan._queuedDownloadTasks.front()->name);
+		Common::Path destPath = Common::Path(ConfMan.get("dlcspath")).appendComponent(gameDir);
+		extractZip(relativeFilePath, destPath);


I think we need to add some kind of error checking there. Imagine that the archive is broken or incomplete or there is no space left on the filesystem for extraction. For the latter, you would need to implement error checking in the dumpArchive() method

sev- · 2023-08-06T19:41:53Z

backends/dlc/scummvmcloud.h

+ *
+ */
+
+#ifndef BACKENDS_DLC_SCUMMVMCLOUD_SCUMMVMCLOUD_H


The correct one is BACKENDS_DLC_SCUMMVMCLOUD_H

Thanks, fixed

sev- · 2023-08-06T19:58:32Z

gui/dlcsdialog.cpp

+	// Set target (Command Receiver) for Command Sender
+	DLCMan.setTarget(this);
+
+	new StaticTextWidget(this, "DownloadGames.Headline", _("Download Freeware Games"));


please add "and demos"

gui/dlcsdialog.cpp

gui/dlcsdialog.h

sev- · 2023-08-06T20:04:19Z

gui/downloaddlcsdialog.h

+
+namespace GUI {
+
+enum {


Please move it to the .cpp

sev- · 2023-08-06T20:05:04Z

gui/launcher.cpp

@@ -255,6 +264,13 @@ void LauncherDialog::build() {
 	// I18N: Button caption. O is the shortcut, Ctrl+O, put it in parens for non-latin (~O~)
 	new ButtonWidget(this, _title + ".OptionsButton", _("Global ~O~ptions..."), _("Change global ScummVM options"), kOptionsCmd, 0, _c("Global ~O~pts...", "lowres"));

+	if (g_system->hasFeature(OSystem::kFeatureDLC)) {
+#if defined(USE_SCUMMVMDLC) && defined(USE_LIBCURL)


What happens on Android? I think you need to show it when the feature is present, not when scummvmdlc is compiled

BLooperZ · 2023-08-07T21:06:16Z

backends/dlc/scummvmcloud.cpp

+	if (json == nullptr || !json->isObject()) {
+		return;
+	}
+	// warning("%s", json->stringify(true).c_str());


perhaps use debug or debugCN from common/debug.h rather than commented out warning

Thanks, done

sev-

Reviewed. We are very close to the merge

sev- · 2023-08-09T16:16:54Z

backends/dlc/scummvmcloud.cpp

+		dataArchive = Common::makeZipArchive(*fs);
+		// dataArchive is nullptr if zip file is incomplete
+		if (dataArchive != nullptr) {
+			dataArchive->dumpArchive(destPath.toString());


Now, since your code is merged, please rebase and add error processing here, e.g. show the user the error messages depending on the returned error.

sev- · 2023-08-09T16:17:53Z

backends/dlc/scummvmcloud.cpp

+	// add a new entry in scummvm config file
+	Common::String domain = EngineMan.generateUniqueDomain(DLCMan._queuedDownloadTasks.front()->gameid);
+	ConfMan.addGameDomain(domain);
+	ConfMan.set("engineid", DLCMan._queuedDownloadTasks.front()->engineid, domain);


For brevity, please put DLCMan._queuedDownloadTasks.front() into a local variable and reuse

sev- · 2023-08-09T16:34:14Z

gui/launcher.cpp

+#if defined(USE_SCUMMVMDLC) && defined(USE_LIBCURL)
+	// I18N: Button browse downloadable games (DLC)
+	new ButtonWidget(this, _title + ".DownloadGamesButton", _("Download Games"), _("Download freeware games for ScummVM"), kDownloadGameCmd);
+#elif defined(USE_DLC)


I do not like this code duplication. You could simplify the thing that when USE_SCUMMVMDLC is defined, g_system->hasFeature(OSystem::kFeatureDLC) is returning true and getDLC() returns ScummVMDLC

sev- · 2023-08-09T16:35:32Z

gui/themes/common/highres_layout.stx

@@ -205,6 +205,9 @@
 					<widget name = 'AddGameButton'
 							type = 'Button'
 					/>


What I am concerned about is that if USE_DLC is not defined, we have a hole between the buttons. Nothing for you to worry about now, I'll see what I can do after testing.

sev-

I have only one small note/enhancement.

I think, we are close to merge this

sev- · 2023-08-15T21:50:41Z

backends/dlc/scummvmcloud.cpp

+}
+
+void ScummVMCloud::startDownloadAsync(const Common::String &id, const Common::String &url) {
+	Common::String localFile = normalizePath(ConfMan.get("dlcspath") + "/" + id, '/');


You need to expose dlcpath in the GUI, Options -> Paths

I have added it. There is, however, a bit of hole/space when USE_DLC is not defined (possibly due to spacing = 10 in layout element inside .stx). I have added the layout (containing widgets) for dlcpath at the end for consistent spacing between the buttons for different paths. The next widget below will have some space before.

For the record, in the launcher, there is no hole/space for the DLC button if USE_DLC is not defined.

…pending DLC downloads

…required

- Developers will need to use --enable-dlc with configure if they want to integrate the DLC downloader. The kFeatureDLC will determine if the target platform is supported - USE_SCUMMVMDLC will be used specifically to compile curl based ScummVMCloud (USE_DLC will be automatically enabled if ScummVMCloud is supported)

…kend

sev- · 2023-09-14T06:23:40Z

Thank you! Merging, you may continue development in-tree

ankushdutt marked this pull request as draft July 3, 2023 11:11

mgerhardy reviewed Jul 4, 2023

View reviewed changes

backends/dlc/dlcmanager.h Outdated Show resolved Hide resolved

ankushdutt force-pushed the gui-download-packages branch 2 times, most recently from ed11ee6 to bd956c2 Compare July 10, 2023 10:14

lephilousophe added the GSoC Part of a Google Summer of Code project label Jul 11, 2023

mgerhardy reviewed Jul 11, 2023

View reviewed changes

ankushdutt force-pushed the gui-download-packages branch 3 times, most recently from 23989d7 to 314ff70 Compare July 21, 2023 08:06

ankushdutt force-pushed the gui-download-packages branch 2 times, most recently from 7533803 to b976a7b Compare July 27, 2023 17:59

sev- requested changes Aug 6, 2023

View reviewed changes

ankushdutt force-pushed the gui-download-packages branch from 186a525 to e20917f Compare August 6, 2023 21:00

BLooperZ reviewed Aug 7, 2023

View reviewed changes

ankushdutt force-pushed the gui-download-packages branch 2 times, most recently from 9aead60 to 551f1ba Compare August 8, 2023 12:26

sev- requested changes Aug 9, 2023

View reviewed changes

ankushdutt force-pushed the gui-download-packages branch 3 times, most recently from 2e5a4ee to a794b0c Compare August 9, 2023 21:27

sev- approved these changes Aug 15, 2023

View reviewed changes

ankushdutt changed the title ~~[WIP] GUI, BACKENDS: Add DLC Downloader~~ GUI, BACKENDS: Add DLC Downloader Aug 16, 2023

ankushdutt marked this pull request as ready for review August 16, 2023 03:21

ankushdutt force-pushed the gui-download-packages branch 2 times, most recently from eced2ec to fe706e9 Compare August 30, 2023 11:51

ankushdutt added 3 commits September 1, 2023 11:48

GUI: Add Download Games button in Launcher

fac8eae

GUI: Setup new dialog for Download Games

ba4a5a6

GUI: Add ListWidget for download games dialog

9bf3b8c

ankushdutt added 27 commits September 1, 2023 11:48

DLC: Wrap ScummVM DLC in configure enabled feature, disabled by default

c7fcfd5

JANITORIAL: Fix defines and code formatting

b2478dd

GUI: Fix crash if a game is downloaded when last item is selected in …

0acb559

…pending DLC downloads

DLC: Use const, uint32 and reference where appropriate

fbe6fa9

BACKENDS: DLC: Refresh DLC list after download is cancelled

24159e3

BACKENDS: Change DLC::DLCDesc to struct, deallocate DLCMan._dlcs

1250e15

JANITORIAL: Move enum to cpp of downloaddlcsdialog.h

0cb978e

JANITORIAL: Change StaticTextWidget's text of dlcsdialog

5d8c970

I18N: Add gui/dlcsdialog and gui/downloaddlcsdialog to POTFILES

a64e5a9

JANITORIAL: Fix formatting and define in scummvmcloud.h

8c96915

JANITORIAL: Remove unneeded includes from store.h

dff57c9

JANITORIAL: Fix indentation in scummvmcloud

15f64e2

BACKENDS: Change return type of getDefaultDLCsPath() to Common::Path

7409ad7

DLC: Use forward declaration where appropriate

90ddf60

JANITORIAL: Use debug instead of commented warning in scummvmcloud.cpp

c5d3d5e

JANITORIAL: Remove DLCMan.init() and its destroy call since it's not …

6e30fb5

…required

JANITORIAL: Remove unneeded includes from dlcmanager.cpp

3f22e72

BACKENDS: DLC: Improve code readability in scummvmcloud.cpp

77c78e3

DLC: Set kFeatureDLC and _dlcStore for ScummVMCloud, Playstore in bac…

18d88ad

…kend

BACKENDS: DLC: Handle errors in ScummVMCloud::extractZip()

387d09c

JANITORIAL: Improve code readability in scummvmcloud.cpp

21cd92c

DLC: Add lowres_layout.stx and generated theme files

409666d

JANITORIAL: Remove unneeded functions from dlcmanager.h

8af36a0

DLC: Remove Playstore code

4a4696e

DLC: Add dlcpath in Global Options

64562da

DLC: Add generated theme files

b986b40

ankushdutt force-pushed the gui-download-packages branch from fe706e9 to b986b40 Compare September 1, 2023 06:18

sev- merged commit 0806d04 into scummvm:master Sep 14, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUI, BACKENDS: Add DLC Downloader #5134

GUI, BACKENDS: Add DLC Downloader #5134

ankushdutt commented Jul 3, 2023

mgerhardy commented Jul 4, 2023

ankushdutt commented Jul 5, 2023

sev- left a comment

sev- Aug 6, 2023

ankushdutt Aug 7, 2023

sev- Aug 6, 2023

ankushdutt Aug 7, 2023

sev- Aug 6, 2023

sev- Aug 6, 2023

ankushdutt Aug 7, 2023

sev- Aug 6, 2023

ankushdutt Aug 7, 2023

sev- Aug 6, 2023

ankushdutt Aug 7, 2023

sev- Aug 6, 2023

BLooperZ Aug 7, 2023

ankushdutt Aug 8, 2023

sev- left a comment

sev- Aug 9, 2023

sev- Aug 9, 2023

sev- Aug 9, 2023

sev- Aug 9, 2023

sev- left a comment

sev- Aug 15, 2023

ankushdutt Aug 16, 2023

sev- commented Sep 14, 2023

GUI, BACKENDS: Add DLC Downloader #5134

GUI, BACKENDS: Add DLC Downloader #5134

Conversation

ankushdutt commented Jul 3, 2023

mgerhardy commented Jul 4, 2023

ankushdutt commented Jul 5, 2023

sev- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sev- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sev- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sev- commented Sep 14, 2023