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

refactor(neon): unify file dialog capabilities #816

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

fritzlimo
Copy link
Contributor

@fritzlimo fritzlimo commented Sep 19, 2023

Fixes #785

Ios support (#755 , #754 ) needs to reimplement this

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I think this should be done differently. I would prefer if the platforms have a method pickFile or something like that and then each platform can implement it in their way.
Right now you would need to just move the first part to Android and the second part to Linux.
@Leptopoda what do you think?

@provokateurin
Copy link
Member

To clarify: The problem here is not wether a feature is available or not, but how we implement it

@Leptopoda
Copy link
Member

I disagree.
While this might make sense for custom implementations we merely call other packages here.
Your approach would also just duplicate a bunch of code as windows and macOS would need to replicate the linux implementation (similar with iOS and Android).
Thus I think the NeonPlatform should only hold some capabilities which then should be used in utility functions that we the framework can provide to others.

We could also just provide a default implementation in NeonPlatform that does not need to be overridden by other platforms but this would only move the utility function into the NeonPlatform with no change to the logic behind it.

@Leptopoda
Copy link
Member

I agree that we could change the naming behind it as we don't want to check whether a platform can use file dialog but rather whether it should use it because of #8 (which we could also mention in the doc comment).

@provokateurin
Copy link
Member

Sure, I'm against duplicating the relevant code. All platforms support this feature in one way or another, so NeonPlatform should indicate which method should be used. We could model it using an enum or another method if there is something better.

@Leptopoda
Copy link
Member

Can you give an example? I can't imagine how this would be any different.
FYI I was thinking on making the NeonPlatform a sealed class anyways.

@Leptopoda
Copy link
Member

@provokateurin bump :)
Otherwise I'd just ask @fritzlimo to change the naming to shouldUseFileDialog.

I mean the doc comment in the platform can also mention the utility function.

@provokateurin
Copy link
Member

I would prefer if the current method was split into two methods (both not exported) and then the platform implementations just call the one they need. I really dislike using the detection logic outside of the platform to decide which method should be used.

@provokateurin
Copy link
Member

Ok let's just rebase and merge this and I will come up with something later. We will likely encounter this problem in the future again, so solving it properly would be nice.

Signed-off-by: Christian Badelt <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
@provokateurin provokateurin merged commit cd59a9b into nextcloud:main Oct 4, 2023
5 checks passed
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.

file utils use NeonPlatform instead of conditional Platform.is*
3 participants