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

Satellite map #116

Merged
merged 12 commits into from
Jun 25, 2023
Merged

Satellite map #116

merged 12 commits into from
Jun 25, 2023

Conversation

shinovon
Copy link
Collaborator

@shinovon shinovon commented Jun 23, 2023

Close #111
image

Copy link
Contributor

Choose a reason for hiding this comment

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

private ChoiceGroup download = new ChoiceGroup(MahoMapsApp.text[54], Choice.POPUP,
			new String[] { MahoMapsApp.text[18], MahoMapsApp.text[55], }, null);

We should rename "Светлая палитра..." to "Выключено"

Copy link
Contributor

Choose a reason for hiding this comment

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

We should rethink this thing completely.

It supposed to be a choice "Off"/"Light scheme"/"Dark scheme"/"Sattelite"/"Hybrid". Before this PR there was 2 options, off and light scheme.

I agree that download switch and layer select should be separate elements, so for the beginning this thing can be changed to "multiple" ChoiceGroup with title as is and single option Enabled. Layer selection is done via switch on 40-41 line.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Feodor0090 dark scheme? Does such a thing exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

So ok, let it be only "Enabled"

Copy link
Contributor

@Feodor0090 Feodor0090 left a comment

Choose a reason for hiding this comment

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

Что-то похожее на это быть и должно, впринципе. Как минимум настройки разгрести. И в прод, наверное...

@@ -101,6 +101,7 @@ private void processFirstLaunch() {
}

protected void destroyApp(boolean arg0) {
Settings.Save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Я даже не знаю до чего бы докопаться, поэтому докопаюсь до того как теперь работают у нас настройки.

До появления экрана с выбором языка: и рмс настроек, и рмс состояния читались после запуска приложения, настройки записывались по необходимости, состояние записывалось при выходе.

Теперь: настройки читаются до старта приложения, записываются при каждом изменении флагов использования и ещё и при выходе.

Как я и сказал, в коде загружаемый слой карты не должен иметь отношения к состоянию карты, и должен быть отдельно. Я вот только начал сомневаться в том как у нас работает это всё. Возможно, слой карты надо оставить полем в канве, и держать его в рмс состояния (вычитывать до вычитки MapState и добавлять в его же объект при записи в рмс)... Туда же можно засунуть флаги, чтоб не дёргать настройки при каждом тычке куда-то.

Но это так, мысли. Можно продолжать впринципе этот цирк игнорировать, от одной лишней переменной мы не умрём.

Одно я скажу точно - здесь этого сейва быть не должно. Перетащи его в обработчик нажатия нуля, как минимум

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

к.

@@ -5,10 +5,12 @@ public class TileId {
public final int x;
public final int y;
public final int zoom;
public final int map;
Copy link
Contributor

Choose a reason for hiding this comment

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

А вот это правильная мысля. Тип подложки в id тайла быть должен.

@@ -575,11 +574,11 @@ private String getUrl(TileId tileId) {
}

private String getFileName(TileId id) {
return localPath + "tile_" + lang + "_" + id.x + "_" + id.y + "_" + id.zoom;
return localPath + getRmsName(id);
}

private String getRmsName(TileId id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

И теперь ещё раз по кругу переименовать. Как - не знаю. getTileName и getTileFileName - уебанство. Так что наверное лучше не трогать...

Copy link
Contributor

Choose a reason for hiding this comment

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

We should rethink this thing completely.

It supposed to be a choice "Off"/"Light scheme"/"Dark scheme"/"Sattelite"/"Hybrid". Before this PR there was 2 options, off and light scheme.

I agree that download switch and layer select should be separate elements, so for the beginning this thing can be changed to "multiple" ChoiceGroup with title as is and single option Enabled. Layer selection is done via switch on 40-41 line.

@shinovon
Copy link
Collaborator Author

я бы все так и оставил и мержнул, мне лично все нравится

@shinovon shinovon marked this pull request as ready for review June 24, 2023 23:09
@Petrprogs
Copy link
Contributor

@Feodor0090 Maybe merge this PR first and then fix all the flaws without haste?

@Feodor0090
Copy link
Contributor

@Feodor0090 Maybe merge this PR first and then fix all the flaws without haste?

Try to google what's the purpose of pull requests to exist

Feodor0090
Feodor0090 previously approved these changes Jun 25, 2023
Copy link
Contributor

@Feodor0090 Feodor0090 left a comment

Choose a reason for hiding this comment

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

хуяк-хуяк и в прод

Comment on lines +463 to +467
case KEY_NUM0:
// layer toggle
Settings.map++;
if (Settings.map >= TilesProvider.tilesUrls.length)
Settings.map = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Для переключения с 2+ слоями (гибрид на подходе)

new String[] { MahoMapsApp.text[18], MahoMapsApp.text[55], }, null);
private ChoiceGroup download = new ChoiceGroup(MahoMapsApp.text[54], Choice.MULTIPLE,
new String[] { MahoMapsApp.text[18] }, null);
private ChoiceGroup map = new ChoiceGroup("Map", Choice.EXCLUSIVE, new String[] { "Scheme", "Satellite" }, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Под загрузкой, а не в самом конце. В идеале - должно быть пунктом в меню, но это потом.

Feodor0090
Feodor0090 previously approved these changes Jun 25, 2023
@shinovon shinovon changed the title Satellite map (my realization) Satellite map Jun 25, 2023
@shinovon shinovon merged commit 564136c into main Jun 25, 2023
2 checks passed
@shinovon shinovon deleted the satellite-map branch June 25, 2023 23:04
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.

Add satelite map
3 participants