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

Bugfix 1114 #1115

Merged
merged 5 commits into from
Feb 3, 2024
Merged

Bugfix 1114 #1115

merged 5 commits into from
Feb 3, 2024

Conversation

MisterChangRay
Copy link
Contributor

@MisterChangRay MisterChangRay commented Feb 1, 2024

implements for #1114

add allow anonymous Access the link share file,

append global setting and onece share settting
image

yes, you also can be setting on share link page, if global setting is off:
image

global settting will be auto override to the share link page setting:
image

@MisterChangRay
Copy link
Contributor Author

it's test pass on windows10&win11&Ubuntu 22.04.3 LTS

@@ -112,6 +113,10 @@ class PersistenceService {
await prefs.setInt(_version, 1);
}

if (prefs.getInt(_anonymousAccess) == null) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to initialize it here.
Initialization is only needed if you don't have a stable default value but I have seen that you have a default value (line 242)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's removed

@@ -233,6 +238,14 @@ class PersistenceService {
await _prefs.setInt(_portKey, port);
}

int getAnonymousAccess() {
return _prefs.getInt(_anonymousAccess) ?? 0;
Copy link
Member

Choose a reason for hiding this comment

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

If you intend to have 3 or more options, then an enum should be used (see getSendMode, setSendMode below).
An int is not type safe enough which could lead to bugs.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking again about this: You commented somewhere that 2 is only temporary. Then we only need to persist 2 states. In this case, a bool should be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking again about this: You commented somewhere that 2 is only temporary. Then we only need to persist 2 states. In this case, a bool should be used here.

if use 2 states, only on/off can be use ;
in the share via link page, there is also a switch for temporay enablement, and it can't affect the global configuration.
so needs 3 states for this functional, globalOn/ globalOff/ onceOn
i will refactor code to enums later,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or use 2 configuration to done this , one use to global config, one use to temporary config

but i think int is better use to, first Bit use to global config, and seceond Bit use to temporary config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking again about this: You commented somewhere that 2 is only temporary. Then we only need to persist 2 states. In this case, a bool should be used here.

i already refactor to bool

use 2 bool variable to control  global setting and temporary setting respectively
@Tienisto
Copy link
Member

Tienisto commented Feb 3, 2024

Thank you

@Tienisto Tienisto merged commit d1d4a58 into localsend:main Feb 3, 2024
1 check failed
@sergd88
Copy link
Contributor

sergd88 commented Feb 4, 2024

In my opinion, this isn't the parameter that most people will use because it's quite specific. Therefore, once again, in my opinion, it would be worth moving it to the "Advanced settings".

@Tienisto
Copy link
Member

Tienisto commented Feb 5, 2024

Thanks @sergd88. I moved it to advanced settings now.

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