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

#506 Add background type Custom #564

Open
wants to merge 2 commits into
base: modified
Choose a base branch
from

Conversation

ravenfeld
Copy link

@ravenfeld ravenfeld commented Jul 2, 2024

I take the liberty of reading a little of the issues and this one was within my technical reach.

I didn't simply replace the value when there is data in the url but added a new entry.
I thought that maybe some people still wanted to keep Esri and use a custom background at the same time.

The only problem is that I find there's a lot of duplicate code for the toogles. Is a refactor acceptable or do you prefer it this way?

Screenshot_20240703_154554

@mnalis
Copy link
Collaborator

mnalis commented Jul 3, 2024

I see the use of such functionality; however, how does quick-button Toggle aerial / map background works then?

Does it always cycle through all 3 of them? Or does it skip custom one if it is empty?

@ravenfeld
Copy link
Author

At the moment, it goes through all 3 states because I haven't redefined the default value for custom.
Custom by default is on ESRI, maybe I should set it to empty and propose that custom is filled in by the user?

@mnalis
Copy link
Collaborator

mnalis commented Jul 3, 2024

Custom by default is on ESRI, maybe I should set it to empty and propose that custom is filled in by the user?

That sound good to me, if the result is that if Custom is empty, in only switches between 2 values (OSM/ESRI)!
(instead of e.g. SCEE still trying to cycle through all 3 values, but displaying empty screen or crashing when reaching empty Custom - which would be bad result, obviously)

@ravenfeld
Copy link
Author

Currently there is a check in the modification of the tile url. It cannot be empty.

So on a first installation :
MAP
ESRI
CUSTOM=ESRI url

Which could be weird, maybe using OSM tiles?

@Helium314
Copy link
Owner

I'm fine with the duplicate code (also: it was already duplicate, so I'm not really in a position to complain).
Though putting it in cycleBackground(prefs) and getBackgroundStringId(background) functions should not be much work.

Though I would like to avoid changes to existing behavior when the user didn't set a custom tile source.
Maybe make it empty by default, allow empty URL, and skip custom from toggle / cycling background if empty.

@ravenfeld
Copy link
Author

"Maybe make it empty by default, allow empty URL, and skip custom from toggle / cycling background if empty."

Can you explain a little more? I don't think I understand.

Make it available in settings but it's empty, so what happens when you return to the view?
Just check if it's alive for the toogle?

With all the changes currently being made to the views, I'm going to wait until it's stabilised before making any changes of my own.

@Helium314
Copy link
Owner

My idea was: when the toggle would go to custom, check whether there actually is a custom background set. If yes, set custom, if not set map.
The URL check can be modified to allow an empty URL.

With all the changes currently being made to the views, I'm going to wait until it's stabilised before making any changes of my own.

Definitely, otherwise it might just be a waste of time.

@Helium314
Copy link
Owner

With all the changes currently being made to the views, I'm going to wait until it's stabilised before making any changes of my own.

Preferences are working again, and I don't intend to migrate them to compose anytime soon. So I'd consider the current state stable (unless changes from upstream make adjustments necessary)

@ravenfeld
Copy link
Author

With all the changes currently being made to the views, I'm going to wait until it's stabilised before making any changes of my own.

Preferences are working again, and I don't intend to migrate them to compose anytime soon. So I'd consider the current state stable (unless changes from upstream make adjustments necessary)

I'm trying to rebase as soon as possible

@Helium314
Copy link
Owner

I'm trying to rebase as soon as possible

No need to hurry, I will not have much time for the next 2 weeks anyway.

@ravenfeld
Copy link
Author

For the moment, this feature no longer exists on the modified branch.

@Helium314
Copy link
Owner

Oh right, it was in the old settings menu (existing SC setting that was just disabled upstream). Probably should be re-added, maybe in display settings?

But switching background using quick settings or the toggle buttons should still work.

@ravenfeld
Copy link
Author

@Helium314 I don't know what option you're talking about.

@Helium314
Copy link
Owner

For the moment, this feature no longer exists on the modified branch.

Where was it, but not any more?

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.

3 participants