-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: implement custom map file support #758
Conversation
ed1ca05
to
43d7b7e
Compare
43d7b7e
to
51e88bc
Compare
51e88bc
to
5c26340
Compare
5c26340
to
632adcc
Compare
remaining bugs: - modification time not being reliable - map name should not come from file name but from style.json - handle FileSystem.documentDirectory being null?
632adcc
to
4215dff
Compare
export function DownloadIcon(props: FontIconProps) { | ||
return <AntDesignIcon {...props} name="download" />; | ||
} |
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.
The Ant variant was the closest to what the designs had - ideally we would stick with the same icon sets across all icons though...
export function convertFileUriToPosixPath(fileUri: string) { | ||
return fileUri.replace(/^file:\/\//, ''); | ||
} | ||
|
||
// TODO: Some overlap with selectFile() from lib/utils but fixes some usage limitations. Ideally use this for everything |
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.
just noting this but if there's an interest in unifying the implementations, I'd rather address in a follow up
function LoadingOverlay() { | ||
return ( | ||
<View pointerEvents="none" style={styles.overlay}> | ||
<View style={[styles.overlay, {opacity: 0.1, backgroundColor: BLACK}]} /> | ||
<Loading size={10} /> | ||
</View> | ||
); | ||
} |
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.
could be useful to extract as a shared component, but would prefer to avoid the extra work if not necessary for now
}, | ||
importErrorDesciption: { | ||
id: 'screens.Settings.MapManagement.BackgroundMaps.importErrorDescription', | ||
defaultMessage: 'Unable to import the file. Please go back and try again.', |
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.
ideally this should include the name of the file or the map name, but given how the error modal works and the context in which the error occurs, it's a bit tricky to get that to work seamlessly. for now, intentionally settling for just a generalization
title={ | ||
selectCustomMapMutation.error || importCustomMapMutation.error | ||
? m.importErrorTitle | ||
: undefined | ||
} | ||
description={ | ||
selectCustomMapMutation.error || importCustomMapMutation.error | ||
? m.importErrorDesciption | ||
: undefined | ||
} |
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.
we only surface a custom title + description in the case of an import error. otherwise just fallback to the default messaging from the component
MapSettings: undefined; | ||
BackgroundMaps: undefined; | ||
BackgroundMapInfo: { | ||
bytesStored: number; | ||
id: string; | ||
styleUrl: string; | ||
name: string; | ||
}; |
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.
artifact of Mapeo Mobile
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.
upon looking at this logic for way too long 😬 ... I think i figured out a possible way to seperate the logic to be a little clearer
// BackgroundMaps.tsx
return (
<ScrollView contentContainerStyle={styles.container}>
<Text style={styles.aboutText}>{t(m.about)}</Text>
<View style={styles.descriptionContainer}>
<Text>{t(m.description1)}</Text>
<Text>{t(m.description2)}</Text>
</View>
{ customMapInfoQuery.isFetching ?
<Loader> : !customMapInfoQuery.data ?
<ChooseMapFile />:
<CustomMapDetails/>
}
{ customMapInfoQuery.isError &&
<>
<Text style={styles.infoLoadErrorText}>
{t(m.customMapInfoLoadError)}
</Text>
<Button
fullWidth
variant="outlined"
onPress={() => {
removeCustomMapMutation.mutate();
}}>
<Text style={styles.removeMapFileButtonText}>
{t(m.removeMapFile)}
</Text>
</Button>
</>
}
</ScrollView>
)
ChooseMapFile
could then just deal with its own loading state. But this would require ChooseMapFile to have its own ErrorModal.
And anytime the maps are fetching, the loader is shown. Since the use can only have 1 custom map we don't need to worry about showing the previous map. This would mean that CustomMapDetails
would not have to have a loading state, because the parent component is dealing with it
I think this would simplifiy the switch statement with if statements inside of it
const DB_DIR_NAME = 'sqlite-dbs' | ||
const CORE_STORAGE_DIR_NAME = 'core-storage' | ||
const CUSTOM_MAPS_DIR_NAME = 'maps' | ||
const DEFAULT_CUSTOM_MAP_FILE_NAME = 'default.smp' |
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.
Just confirming that this requires the developer to know the names of the directories? If so, that seems quite fragile, maybe something to consider refactoring at the UK retreat.
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.
yeah there's some duplication between frontend and backend in terms of knowing where this directory is. However, there's no good way to share code between the two unless you involve an unnecessary IPC roundtrip, which I'd rather avoid in this case.
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 posterity: the way this custom map feature works is the following:
- The frontend is responsible for moving a file to and from an app-specific directory on the device. In this case, we the android "document directory" which is something like
file:///data/user/0/com.comapeo.dev/files/
and make amaps/default.smp
there. - The backend is configured to use the path in (1) as the path to watch for determining if there's a custom map file that can be used for the map server.
src/frontend/hooks/server/maps.ts
Outdated
export function useSelectCustomMapFile() { | ||
return useMutation({ | ||
mutationFn: () => { | ||
return selectFile({ | ||
extensionFilters: ['smp'], | ||
}); | ||
}, | ||
}); | ||
} |
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.
I think based on how simple this function is, we should just have a useSelectCustomFile(extenstionfilters?:[string])
, and the consuming component can set the extensions. What do you think?
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.
addressed via dcac9e0
Think ideally this stuff in useSelectFileAndImportConfig.ts
also lives in the file I created here, but would consider that more of a follow-up item
refresh(); | ||
queryClient.invalidateQueries({ | ||
queryKey: [MAPS_QUERY_KEY], | ||
}); |
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.
I don't fully understand the need for the refresh token? From a surface perspective it looks like it is doing the same thing as invalidating the query
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.
yeah the tricky part here is that the invalidation we need is controlled by the consuming map view, not react query. for example:
- We have an initial value of:
http://localhost:8000/maps/style.json
. This is used by the map view, which makes the http request internally to fetch a response. let's say{ "name": "my-map" }
for the sake of the example. - We do something that should change what style.json we get back in 1 (such as add or remove a map).
- We invalidate the query, but that isn't enough in some cases because you can still get the same value of
http://localhost:8000/maps/style.json
, in which case the mapview won't actually get a new response from that URL because of HTTP caching.
In order to actually force the mapview to fully fetch the response of the same url and ignore the cache, we introduce the refresh token in the form of a url query string, which counts as a new url, even though the query string goes unused by the server that we're calling. we did something similar in Mapeo Mobile for other http resources that would have the same issue (e.g. here)
<BottomSheetModal | ||
ref={removeMapBottomSheet.sheetRef} | ||
isOpen={removeMapBottomSheet.isOpen}> | ||
<BottomSheetModalContent | ||
icon={<ErrorSvg />} | ||
title={t(m.deleteCustomMapTitle)} | ||
description={ | ||
t(m.deleteCustomMapDescription) + '\n\n' + t(m.cannotBeUndone) | ||
} | ||
buttonConfigs={[ | ||
{ | ||
dangerous: true, | ||
variation: 'filled', | ||
text: t(m.deleteMapButtonText), | ||
icon: <MaterialIcon size={30} name="delete" color={WHITE} />, | ||
onPress: () => { | ||
removeCustomMapMutation.mutate(); | ||
removeMapBottomSheet.closeSheet(); | ||
}, | ||
}, |
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.
would it not be simpler to have 1 bottom sheet modal, where the contents are conditionally rendered?
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.
in this case i would argue against it for the following reasons:
-
One is a full-sized sheet and the other isn't. If i remember correctly, we had issues with dynamically switching between full-size and non-full-size (some issue with the component implementation)
-
It provides a more explicit separation of states that can occur. Part of this is due to a limitation of the how the bottom sheet component is designed, but i'd rather avoid code that does something like
isOpen={mapAddedOpen || mapRemovedOpen}
, because then there's a greater risk of introducing or seeing the wrong sheet content, unless you're careful about the checks you make in it.
if (customMapInfoQuery.isFetching) { | ||
renderedMapInfo = customMapInfoQuery.data ? ( | ||
<CustomMapDetails | ||
loading | ||
name={customMapInfoQuery.data.name} | ||
dateAdded={customMapInfoQuery.data.created} | ||
size={customMapInfoQuery.data.size} | ||
onRemove={() => { | ||
removeMapBottomSheet.openSheet(); | ||
}} | ||
/> | ||
) : ( | ||
<Loading size={10} /> | ||
); |
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.
if its fetching, shouldnt we always be showing a loading state? Under the assumption that there can only be 1 background map, swr doesn't apply here
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.
To clarify: this is handling the case where a map currently exists and we're doing something that would trigger another fetch (e.g. removing or updating).
It is showing a loading state (see the loading
prop in the CustomMapDetails
) but it's doing a more visually graceful loading state. I found it very jarring for the information to just suddenly disappear, turn into the three dots, and then into some other UI, so I implemented a simple overlay that prevents less layout shift in this specific case.
It looks like this, which may look weird when seeing as a screenshot, but I argue that it's preferable to just showing the dots:
Haven't tried out your suggestion just yet, but I'm a little hesitant about introducing an additional query "boundary" when it may not be necessary to. I usually prefer to keep all of the query logic at the same level and just have the rendered components be pretty dumb. What I currently like about what I have implemented is that it clearly handles all of the states that this UI can end up in:
While the implementation doesn't look pretty, I don't find it overly difficult to follow. Just happens to be a consequence of how to work with react + react query. Even if I move a lot of the logic to another nested component, I'd still probably end up with the switch statement approach, even though it may be slightly less complicated. Overall I find it easier to approach building the UI based on react query's |
here's a first attempt at what you suggested that takes into the various states that I mentioned (no additional component separation just yet): <ScrollView contentContainerStyle={styles.container}>
<Text style={styles.aboutText}>{t(m.about)}</Text>
<View style={styles.descriptionContainer}>
<Text>{t(m.description1)}</Text>
<Text>{t(m.description2)}</Text>
</View>
{/* Think your suggestion was to make this a separate component */}
{customMapInfoQuery.status === 'pending' ? (
<Loading size={10} />
) : customMapInfoQuery.data ? (
<CustomMapDetails
loading={customMapInfoQuery.isFetching}
name={customMapInfoQuery.data.name}
dateAdded={customMapInfoQuery.data.created}
size={customMapInfoQuery.data.size}
onRemove={() => {
removeMapBottomSheet.openSheet();
}}
/>
) : customMapInfoQuery.isFetching ? (
<Loading size={10} />
) : (
<ChooseMapFile
onChooseFile={() => {
selectCustomMapMutation.mutate(undefined, {
onSuccess: asset => {
if (!asset) return;
importCustomMapMutation.mutate(
{
uri: asset.uri,
},
{
onError: () => {
FileSystem.deleteAsync(asset.uri, {
idempotent: true,
}).catch(noop);
},
onSuccess: () => {
mapAddedBottomSheet.openSheet();
},
},
);
},
});
}}
/>
)}
{customMapInfoQuery.status === 'error' && (
<>
<Text style={styles.infoLoadErrorText}>
{t(m.customMapInfoLoadError)}
</Text>
<Button
fullWidth
variant="outlined"
onPress={() => {
removeCustomMapMutation.mutate();
}}>
<Text style={styles.removeMapFileButtonText}>
{t(m.removeMapFile)}
</Text>
</Button>
</>
)}
</ScrollView> Aside from the error stuff, which I agree can just be inlined as you suggested, I don't know how I feel about this. EDIT: The more I look at it, the more okay I am with it. Definitely appreciate that it's less code in general. just find it a little harder to understand that ternary, which I could break up into a switch statement or variable in the render body, but that would be closer to the initial criticisim 😄 |
Alright did a little cleanup that moves towards what you suggested and I'm feeling much better about it: <ScrollView contentContainerStyle={styles.container}>
<Text style={styles.aboutText}>{t(m.about)}</Text>
<View style={styles.descriptionContainer}>
<Text>{t(m.description1)}</Text>
<Text>{t(m.description2)}</Text>
</View>
<CustomMapInfoSection
onChooseFile={() => {
selectCustomMapMutation.mutate(undefined, {
onSuccess: asset => {
if (!asset) return;
importCustomMapMutation.mutate(
{
uri: asset.uri,
},
{
onError: () => {
FileSystem.deleteAsync(asset.uri, {
idempotent: true,
}).catch(noop);
},
onSuccess: () => {
mapAddedBottomSheet.openSheet();
},
},
);
},
});
}}
onRemoveMap={() => {
removeMapBottomSheet.openSheet();
}}
/>
{customMapInfoQuery.status === 'error' && (
<>
<Text style={styles.infoLoadErrorText}>
{t(m.customMapInfoLoadError)}
</Text>
<Button
fullWidth
variant="outlined"
onPress={() => {
removeCustomMapMutation.mutate();
}}>
<Text style={styles.removeMapFileButtonText}>
{t(m.removeMapFile)}
</Text>
</Button>
</>
)}
</ScrollView> where function CustomMapInfoSection({
onChooseFile,
onRemoveMap,
}: {
onChooseFile: () => void;
onRemoveMap: () => void;
}) {
const customMapInfoQuery = useGetCustomMapInfo();
if (customMapInfoQuery.status === 'pending') {
return <Loading size={10} />;
}
if (customMapInfoQuery.data) {
return (
<CustomMapDetails
loading={customMapInfoQuery.isFetching}
name={customMapInfoQuery.data.name}
dateAdded={customMapInfoQuery.data.created}
size={customMapInfoQuery.data.size}
onRemove={onRemoveMap}
/>
);
}
return customMapInfoQuery.isFetching ? (
<Loading size={10} />
) : (
<ChooseMapFile onChooseFile={onChooseFile} />
);
} Thanks for bearing with me! Now the question is: is this an improvement over what I currently have implemented? Would say that it's maybe enough of an improvement to commit to - thanks for the suggestion and making me think harder about this! EDIT: Part of me wants to include the error rendering in that |
This implementation on a surface level looks much cleaner and is defintely easier to follow. |
Heads up: I updated core and IPC deps with the fix to the issue mentioned in the PR description via b2c015a |
"@comapeo/core": "2.0.1", | ||
"@comapeo/ipc": "2.0.2", |
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.
just highlighting that we need this update to core to fix an issue that happens when removing a custom map.
@@ -108,14 +108,15 @@ | |||
"tiny-typed-emitter": "^2.1.0", | |||
"uint8array-extras": "^0.5.0", | |||
"utm": "^1.1.1", | |||
"valibot": "^0.42.1", |
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.
I'm introducing this dep for response body validation, which is used when interfacing with the map server
const DB_DIR_NAME = 'sqlite-dbs' | ||
const CORE_STORAGE_DIR_NAME = 'core-storage' | ||
const CUSTOM_MAPS_DIR_NAME = 'maps' | ||
const DEFAULT_CUSTOM_MAP_FILE_NAME = 'default.smp' |
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 posterity: the way this custom map feature works is the following:
- The frontend is responsible for moving a file to and from an app-specific directory on the device. In this case, we the android "document directory" which is something like
file:///data/user/0/com.comapeo.dev/files/
and make amaps/default.smp
there. - The backend is configured to use the path in (1) as the path to watch for determining if there's a custom map file that can be used for the map server.
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.
Nice job! seems to be working. A few small comments, but non-blocking
src/frontend/hooks/server/maps.ts
Outdated
export function useSelectFile() { | ||
return useMutation({ | ||
mutationFn: (opts: Parameters<typeof selectFile>[0]) => { | ||
return selectFile(opts); | ||
}, | ||
}); | ||
} |
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.
Do we need this twice?
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.
oh no we don't - i copied this to a different file but forgot to actually use that one. Removed this one via 5a702b4
removeCustomMapMutation.mutate(); | ||
removeMapBottomSheet.closeSheet(); |
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.
Any concerns about not having a loading state
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.
not super concerned because this happens so quickly and also we have a different modal for an error that may occur. Added a small change anyways just in case though, via b79e78d
ref={mapAddedBottomSheet.sheetRef} | ||
isOpen={mapAddedBottomSheet.isOpen}> | ||
<BottomSheetModalContent | ||
icon={<GreenCheckSvg />} |
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.
this should have a marginTop of 80
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.
yeah this ideally is fixed in the BottomSheetModalContent
component but addressed here as a one-off for now (since we probably do the same in other places). See e65020c
<ScrollView> | ||
<List> | ||
<ListItem | ||
onPress={() => { | ||
navigation.navigate('BackgroundMaps'); | ||
}}> | ||
<ListItemText primary={t(m.backgroundMaps)} /> | ||
</ListItem> | ||
</List> | ||
</ScrollView> |
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.
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.
seems somewhat questionable for that design spec to apply to this page, especially when compared to other settings-like pages.
App settings:
Screen in question (with your suggestion):
My guess is that either:
- the app settings page needs to be updated too
- the design spec does not apply to pages like these
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.
the app settings page needs to be updated too
ah yeah it this seems likely when looking at the screenshot in #622
I think for the sake of consistency, I'm going to avoid making this change so that it matches the app settings layout. we can address both of those in a follow up (since there are a good number of design changes needed anyways)
Stacked on #796Closes #622
Closes #623
Closes #624
Refer to the designs in Figma instead of the screenshots from the issues, since they're a bit outdated.
Implementation details:
the error state in the case of validly named file that is actually invalid as a custom map was not detailed in the design, so I had to implement that using my own judgment.EDIT: Mostly eliminated by actually verifying the file (see 5284dc3), although I kept the UI pieces so that we handle the super edge case better in case it really does happen for whatever reason.for trying it out, you can use fixtures from https://github.com/digidem/styled-map-package/tree/main/test/fixtures
Known issues:
Removing the custom map may not properly update the rendered map (should eventually be fixed in core via fix: support maps that may be removed or replaced in map server plugin comapeo-core#906) - will be resolved before merging. The demo video is using the changes that fix this issue but they're not in the PR yet, so if you're trying to test it yourself locally it probably won't work.EDIT: no longer an issue with b2c015aPotential loading issue with download icon, causing a visual flicker when initially loading. I'm not sure if this is a dev-only issue but wondering how to fix 🤔
custom-map.mp4