[feat-75] support drag and drop for map zip files #575

silentrald · 2024-09-05T16:18:24Z

Scope

closes #75

Implementation

Implemented drag and drop for map zip files.

Added Dropzone component to support drag and drop of files.

Created processZip in zip helper file to safely handle zip files, although SonarCloud doesn't seem to like the handling.

Added LocalMapsManagerService.importMap to import the zip file and copied almost the same code in downloadMap for pathing and caching.

Screenshots

How to Test

Drag and drop the following files:

Valid map zip file
Invalid zip file
Any non-zip file

Check if it will be installed in either the specific version folder or shared folder as well.

Check if switching to the playlist tab is not affected by the dropzone.

New Resource Text

List of added resource text

notifications.maps.import-map.titles.success: Map import complete
notifications.maps.import-map.titles.error: An error occurred while importing the map
notifications.maps.import-map.msgs.success: Imported all maps successfully
notifications.maps.import-map.msgs.success: Imported some maps successfully
notifications.maps.import-map.msgs.only-accept-zip: Only zip files are supported
notifications.maps.import-map.msgs.not-found-zip: Zip file does not exists
notifications.maps.import-map.msgs.invalid-zip: Zip file does not contain any dat files

Emoji Guide

For reviewers: Emojis can be added to comments to call out blocking versus non-blocking feedback.

E.g: Praise, minor suggestions, or clarifying questions that don’t block merging the PR.

🟢 Nice refactor!

🟡 Why was the default value removed?

E.g: Blocking feedback must be addressed before merging.

🔴 This change will break something important


Blocking	🔴 ❌ 🚨	RED
Non-blocking	🟡 💡 🤔 💭	Yellow, thinking, etc
Praise	🟢 💚 😍 👍 🙌	Green, hearts, positive emojis, etc

Zagrios · 2024-09-28T08:57:06Z

src/main/services/additional-content/maps/local-maps-manager.service.ts

+            log.info(`Importing map "${zipPath}" to "${mapPath}"`);
+
+            const zip = await JSZip.loadAsync(await readFile(zipPath));
+            const infoFile = zip.file("Info.dat");


⚠️ Info.dat can also exist as info.dat

Zagrios · 2024-09-28T09:03:46Z

src/main/services/additional-content/maps/local-maps-manager.service.ts

@@ -306,6 +309,85 @@ export class LocalMapsManagerService {
        return null;
    }

+    public importMaps(zipPaths: string[], version?: BSVersion): Observable<Progression> {
+        const progress: Progression<string> = {


This function should handle zips containing multiple maps, not just one zip per map.
It should be compatible with the export maps feature of BSM, which creates a single zip file for all the selected maps.

Is there a way to currently verify that the exportMaps zip file is generated by BSM? Something like a metadata within the zip file.

Also the only problem that I can see here is the zip bomb check I've implemented in processZip. Where there is a limit to the file count and file size. We can add that information in within the metadata I guess to bypass the check and a warn users about downloading other users exportMaps zip files.

We can add a metadata file to the generated zip, but users can totally create zips that contain multiple maps on their own for plenty of reasons.

We can maybe split the zip verification to another ipc, and warn the user with a modal if the zip exeed a certain size or a certain amount of maps 🤔

Or we could let the user be responsible for what kind of zips they are importing (I mean, if they want to import 1TB of maps, they should be able to do it)

We can add a metadata file to the generated zip, but users can totally create zips that contain multiple maps on their own for plenty of reasons.

Yeah fair point, the metadata would be either useless or redundant in any case. Might just do a simple verification on each folder on depth 1 if they contain the "I/info.dat" file.

We can maybe split the zip verification to another ipc, and warn the user with a modal if the zip exeed a certain size or a certain amount of maps 🤔
Or we could let the user be responsible for what kind of zips they are importing (I mean, if they want to import 1TB of maps, they should be able to do it)

I think the latter is probably the better option since the user should be responsible for what the zip files that are being imported is.

I've implemented that check is because sonarcloud suggested it so might ignore that in the future in any case.

Yeah, I think SonarCloud's suggestion is great when it’s for a website, but it doesn’t really apply to BSM

Zagrios · 2024-09-28T09:08:21Z

src/main/services/additional-content/maps/local-maps-manager.service.ts

+                        );
+                    }
+                }
+                observer.complete();


Prefer placing the .complete() in the finally block and add an .error() in the catch of the promise.
This way, it will handle all potential errors that are not caught by your try/catch block.
(Even though, I agree, in the current state, unhandled errors are shouldn't possible)

Zagrios · 2024-09-28T09:17:23Z

src/main/services/additional-content/maps/local-maps-manager.service.ts

+                        await this.importMap(zipPath, version);
+                    } catch (error: any) {
+                        log.error(`Could not import "${zipPath}"`, error);
+                        this.ipc.send<{map: string, error: string}>(


Its preferable to remove this ipc.send cause' it can spam the user if all dropped zips are invalid.
Just handle errors on the frontend as normally and show a warning if the progression does not reach 100%
(This is even truer when this function will handle zips of multiple maps)

Zagrios · 2024-09-28T09:41:25Z

src/renderer/services/maps-manager.service.ts

    private constructor() {
        this.ipcService = IpcService.getInstance();
        this.modal = ModalService.getInstance();
        this.progressBar = ProgressBarService.getInstance();
        this.notifications = NotificationService.getInstance();
        this.config = ConfigurationService.getInstance();
        this.linker = VersionFolderLinkerService.getInstance();
+
+        this.ipcService.watch<{map: BsmLocalMap, version?: BSVersion}>("map-imported")


You don't need these watchers.
I will deprecate the watch function in the future.
Using it too much overcomplicates the code; the backend should never send random events like this. It should only send events when a function is called, until the end of the function’s execution.

In your importMaps, the "bs-maps.import-maps" should return a Progression<BsmLocalMap>, and then you can do something like this:

sendV2("bs-maps.import-maps", {...args}).pipe( tap( progress => importListerners.foreach(listener => listener(version, progress.data)) )

Got this, might just do this on import-maps and not on download-maps. I can do the refactoring on download maps once I get this PR merged.

Let me this 👌, there is few more places that still use this function, I will try to remove them all today

Zagrios · 2024-09-28T09:43:19Z

src/renderer/services/maps-manager.service.ts

+            });
+
+        // NOTE: Generic notify call from server side
+        this.ipcService.watch<{map: string, error: string}>("map-not-imported")


Same here, but this should be removed anyway due to the spam of notifications

Zagrios · 2024-09-28T09:46:46Z

src/renderer/services/maps-manager.service.ts

+    }
+
+    public removeImportListener(listener: (map: BsmLocalMap, version?: BSVersion) => void): void {
+        const index = this.importListeners.indexOf(listener);


I wonder what happen if the same callback is added multiple time 🤔

Like:

const cb = () => {}; addImportListener(cb); addImportListener(cb); addImportListener(cb); removeImportListener(cb);

Maybe a Set of callbacks would work better here instead of an array 🤔
I let you test that 😉

Yeah might be better to do a Set in any case, but we'll see if that works.

Actually there was a bug is this code as well, where the listeners weren't removed properly. Not sure if the fix should be here since the PR might take some more time to get merged, but the snippet bugged code is here.

In master currently
local-maps-list-panel.component.tsx

useEffect(() => { if (isActiveOnce) { loadMaps(); } return () => { setMaps(null); loadPercent$.next(0); subs.forEach(s => s.unsubscribe()); mapsDownloader.removeOnMapDownloadedListener(loadMaps); // should not be loadMaps }; }, [isActiveOnce, version, linked]); useEffect(() => { if(isActiveOnce){ // The listener that was being added multiple times mapsDownloader.addOnMapDownloadedListener((map, targetVersion) => { if (!equal(targetVersion, version)) { return; } setMaps((maps ? [map, ...maps] : [map])); }); } return () => { mapsDownloader.removeOnMapDownloadedListener(loadMaps); // should not be loadMaps } }, [isActiveOnce, version, maps])

👀 Yeah I will fix that in on master 😅

silentrald · 2024-09-29T10:44:37Z

@Zagrios Side tangent, seems that the call from observer.error shows the call stack error in the renderer. Seems the same as well for the other sendV2 <-> Observer interaction in the codebase as well. Might be a bug in the integration? But might check with you if this is expected.

Zagrios · 2024-09-29T10:47:38Z

@Zagrios Side tangent, seems that the call from observer.error shows the call stack error in the renderer. Seems the same as well for the other sendV2 <-> Observer interaction in the codebase as well. Might be a bug in the integration? But might check with you if this is expected.

Yeah you need to handle the error in the frontend as well

silentrald · 2024-09-29T10:55:23Z

Yeah you need to handle the error in the frontend as well

Already did in the latest commit, the notification popup shows in the background when I close the error stack in the renderer so I'm not sure what part I've missed 🤔.

bs-manager/src/renderer/services/maps-manager.service.ts

Lines 163 to 215 in 1ccaad2

    
               const importProgress$ = this.ipcService.sendV2( 
        
                   "bs-maps.import-maps", 
        
                   { paths, version } 
        
               ).pipe( 
        
                   tap(progress => { 
        
                       if (!progress.data) { 
        
                           importTotal = progress.total; 
        
                           return; 
        
                       } 
        
                       ++importCount; 
        
                       mapBatch.push(progress.data); 
        
                       if (mapBatch.length >= this.IMPORT_BATCH_SIZE) { 
        
                           const currentBatch = mapBatch.splice(0, this.IMPORT_BATCH_SIZE); 
        
                           this.importListeners.forEach( 
        
                               listeners => listeners(currentBatch, version) 
        
                           ); 
        
                       } 
        
                   }), 
        
                   map(progress => ({ 
        
                       progression: (progress.current / progress.total) * 100, 
        
                       label: progress.data?.songDetails?.name 
        
                   } as ProgressionInterface)), 
        
                   share(), 
        
               ); 
        
               this.progressBar.show(importProgress$, true); 
        
               await lastValueFrom(importProgress$); 
        
               if (mapBatch.length > 0) { 
        
                   this.importListeners.forEach(listeners => listeners(mapBatch, version)); 
        
               } 
        
               if (importCount === importTotal) { 
        
                   this.notifications.notifySuccess({ 
        
                       title: "notifications.maps.import-map.titles.success", 
        
                       desc: "notifications.maps.import-map.msgs.success", 
        
                   }); 
        
               } else if (importCount > 0) { 
        
                   this.notifications.notifySuccess({ 
        
                       title: "notifications.maps.import-map.titles.success", 
        
                       desc: "notifications.maps.import-map.msgs.some-success", 
        
                   }); 
        
               } 
        
           } catch (error: any) { 
        
               this.notifications.notifyError({ 
        
                   title: "notifications.maps.import-map.titles.error", 
        
                   desc: ["invalid-zip"].includes(error?.code) 
        
                       ? `notifications.maps.import-map.msgs.${error.code}` 
        
                       : "misc.unknown" 
        
               }); 
        
           } finally { 
        
               this.progressBar.hide(true); 
        
           }

Zagrios · 2024-09-29T11:15:02Z

Yeah you need to handle the error in the frontend as well

Already did in the latest commit, the notification popup shows in the background when I close the error stack in the renderer so I'm not sure what part I've missed 🤔.

bs-manager/src/renderer/services/maps-manager.service.ts

Lines 163 to 215 in 1ccaad2

const importProgress$ = this.ipcService.sendV2(

"bs-maps.import-maps",

{ paths, version }

).pipe(

tap(progress => {

if (!progress.data) {

importTotal = progress.total;

return;

}

++importCount;

mapBatch.push(progress.data);

if (mapBatch.length >= this.IMPORT_BATCH_SIZE) {

const currentBatch = mapBatch.splice(0, this.IMPORT_BATCH_SIZE);

this.importListeners.forEach(

listeners => listeners(currentBatch, version)

);

}

}),

map(progress => ({

progression: (progress.current / progress.total) * 100,

label: progress.data?.songDetails?.name

} as ProgressionInterface)),

share(),

);

this.progressBar.show(importProgress$, true);

await lastValueFrom(importProgress$);

if (mapBatch.length > 0) {

this.importListeners.forEach(listeners => listeners(mapBatch, version));

}

if (importCount === importTotal) {

this.notifications.notifySuccess({

title: "notifications.maps.import-map.titles.success",

desc: "notifications.maps.import-map.msgs.success",

});

} else if (importCount > 0) {

this.notifications.notifySuccess({

title: "notifications.maps.import-map.titles.success",

desc: "notifications.maps.import-map.msgs.some-success",

});

}

} catch (error: any) {

this.notifications.notifyError({

title: "notifications.maps.import-map.titles.error",

desc: ["invalid-zip"].includes(error?.code)

? `notifications.maps.import-map.msgs.${error.code}`

: "misc.unknown"

});

} finally {

this.progressBar.hide(true);

}

The error comes from this line this.progressBar.show(importProgress$, true); Since the progressBarService shares the same observer, it also receives the error from it.
You need to derive the main observer to create another one that doesn't receive any errors.

You can check the handleDownload() function in oculus-downloader.service.ts

silentrald · 2024-09-29T11:40:06Z

Ahhh gotcha, will amend the last commit so that the PR should be ready for a re-review.

Zagrios · 2024-10-08T15:42:12Z

@silentrald I've just pushed some fixes, but I have to go.
I'm still encountering an error when I drop my zip file, as it doesn't find any Info.dat (see the gif).

I get this error message:

[1] 17:37:30.357 > Zip file "C:\Users\Mathieu\Desktop\1.37.3Maps.zip" does not contain any "Info.dat" file
[1] 17:37:30.358 > Error: No "Info.dat" file located in any of the zip files

The const files = zip.file(this.INFO_DAT_REGEX); results as empty array.

silentrald · 2024-10-08T16:25:07Z

@Zagrios might be related to the path separator. Not sure if Windows uses / or \, couldn't check this on my linux machine, though let me try to fix this in a Windows VM.

fixed issue in map downloads where download listener in MapsDownloaderService was not being removed

sonarcloud · 2024-10-08T17:47:29Z

Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarCloud

silentrald · 2024-10-08T17:50:46Z

@Zagrios got it to work on Windows.

Lost your commit though when I did a full rebase but I tried to reapply it on commit 615ac04. Might need to double check that, if I missed some lines, you can cherry pick the commit on your local to this PR.

silentrald force-pushed the feat/75 branch 2 times, most recently from e089bcc to 9eef153 Compare September 9, 2024 16:26

silentrald force-pushed the feat/75 branch from 9eef153 to 4ac90b2 Compare September 23, 2024 15:07

Zagrios requested changes Sep 28, 2024

View reviewed changes

silentrald force-pushed the feat/75 branch from a7518c3 to 1ccaad2 Compare September 29, 2024 10:33

silentrald force-pushed the feat/75 branch 2 times, most recently from 81615b6 to ebfe849 Compare September 29, 2024 12:15

silentrald added 7 commits October 9, 2024 01:06

[feat-75] support drag and drop for map zip files

2d4da9b

[feat-75] support multiple drag and drop zip files for maps

707e4f2

fixed issue in map downloads where download listener in MapsDownloaderService was not being removed

[feat-75] add translations

be42f42

[feat-75] applied PR comments 1

e2d2144

[feat-75] support import for maps exported by BSM

f9bca2a

[feat-75] use map hash for comparing

05e3493

[feat-75] fix issue with Windows pathing on jsZip pattern

7e851c5

silentrald force-pushed the feat/75 branch from bc135e4 to 2b07f7a Compare October 8, 2024 17:32

[feat-75] added fix on rebasing 1

615ac04

silentrald force-pushed the feat/75 branch from 2b07f7a to 615ac04 Compare October 8, 2024 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat-75] support drag and drop for map zip files #575

[feat-75] support drag and drop for map zip files #575

silentrald commented Sep 5, 2024 •

edited

Loading

Zagrios Sep 28, 2024

Zagrios Sep 28, 2024 •

edited

Loading

silentrald Sep 28, 2024

Zagrios Sep 28, 2024

silentrald Sep 28, 2024

Zagrios Sep 28, 2024

Zagrios Sep 28, 2024 •

edited

Loading

Zagrios Sep 28, 2024

Zagrios Sep 28, 2024

silentrald Sep 28, 2024

Zagrios Sep 28, 2024 •

edited

Loading

Zagrios Sep 28, 2024

Zagrios Sep 28, 2024 •

edited

Loading

silentrald Sep 28, 2024

Zagrios Sep 28, 2024

silentrald commented Sep 29, 2024

Zagrios commented Sep 29, 2024

silentrald commented Sep 29, 2024

Zagrios commented Sep 29, 2024

silentrald commented Sep 29, 2024

Zagrios commented Oct 8, 2024

silentrald commented Oct 8, 2024

sonarcloud bot commented Oct 8, 2024

silentrald commented Oct 8, 2024

[feat-75] support drag and drop for map zip files #575

Are you sure you want to change the base?

[feat-75] support drag and drop for map zip files #575

Conversation

silentrald commented Sep 5, 2024 • edited Loading

Scope

Implementation

Screenshots

How to Test

New Resource Text

Emoji Guide

Choose a reason for hiding this comment

Zagrios Sep 28, 2024 • edited Loading

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

Zagrios Sep 28, 2024 • edited Loading

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

Zagrios Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zagrios Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silentrald commented Sep 29, 2024

Zagrios commented Sep 29, 2024

silentrald commented Sep 29, 2024

Zagrios commented Sep 29, 2024

silentrald commented Sep 29, 2024

Zagrios commented Oct 8, 2024

silentrald commented Oct 8, 2024

sonarcloud bot commented Oct 8, 2024

Quality Gate failed

silentrald commented Oct 8, 2024

silentrald commented Sep 5, 2024 •

edited

Loading

Zagrios Sep 28, 2024 •

edited

Loading

Zagrios Sep 28, 2024 •

edited

Loading

Zagrios Sep 28, 2024 •

edited

Loading

Zagrios Sep 28, 2024 •

edited

Loading