[core] Implement products configuration #9695

kirylkaveryn · 2024-11-20T08:48:55Z

kirylkaveryn commented

2024-11-20 08:48:55 +00:00

This PR adds support for the product's config to the core.

fetch and parse ProductsConfig json
save it to the separate file "products_settings.json" (when the new incoming setting config during the map update doesn't contain the productsConfig the file will be deleted)
add unit tests for servers config and products config
add conditions for showing/not showing the products section to the framework:

Whether the connection is enabled.
Whether donations are enabled at all.
Whether the map for the selected Point of Interest (POI) is downloaded.
Timeout since the first launch.
Timeout since the app was closed + timeout since the last tap on the donation button.

Timeout are different for the debug and release schemes.

This PR adds support for the product's config to the core. 1. fetch and parse ProductsConfig json 2. save it to the separate file "products_settings.json" (when the new incoming setting config during the map update doesn't contain the `productsConfig` the file will be deleted) 3. add unit tests for servers config and products config 4. add conditions for showing/not showing the products section to the framework: - Whether the connection is enabled. - Whether donations are enabled at all. - Whether the map for the selected Point of Interest (POI) is downloaded. - Timeout since the first launch. - Timeout since the app was closed + timeout since the last tap on the donation button. Timeout are different for the debug and release schemes.

biodranik (Migrated from github.com) reviewed 2024-11-20 08:48:55 +00:00

vng (Migrated from github.com) reviewed 2024-11-20 10:46:22 +00:00

vng (Migrated from github.com) left a comment

Do not mix settings and products. Keep all "products" stuff in products.cpp (singleton).
servers_list operates with a string without json parsing.

Details below.

- Do not mix settings and products. Keep all "products" stuff in products.cpp (singleton). - servers_list operates with a string without json parsing. Details below.

platform/servers_list.cpp

					
				@ -38,7 +43,13 @@ std::optional<MetaConfig> ParseMetaConfig(std::string const & jsonStr)

				          outMetaConfig.m_settings[key] = valueStr;

				      }

vng (Migrated from github.com) commented

2024-11-20 10:39:32 +00:00

We are doing unnecessary transformations here:

m_productsConfig should be a simple string
outMetaConfig.m_productsConfig = json_dumps(product, JSON_ENCODE_ANY)
thus avoiding servers_list dependency from products

We are doing unnecessary transformations here: - m_productsConfig should be a simple string - outMetaConfig.m_productsConfig = json_dumps(product, JSON_ENCODE_ANY) - thus avoiding servers_list dependency from products

platform/settings.hpp

					
				@ -76,32 +77,8 @@ public:

				  void EnterForeground();

				  void EnterBackground();

vng (Migrated from github.com) commented

2024-11-20 10:32:37 +00:00

first launch or sessions count?

vng (Migrated from github.com) commented

2024-11-20 10:41:41 +00:00

Make ProductSettings like singleton and remove these functions from settings. Will use separate ProductSettings::Instance()

storage/map_files_downloader.cpp

vng (Migrated from github.com) commented

2024-11-20 10:42:15 +00:00

ProductSettings::Instance().Update(metaConfig.m_productsConfig); // passing a json string

vng (Migrated from github.com) reviewed 2024-11-20 10:53:17 +00:00

map/framework.cpp

					
				@ -3316,0 +3361,4 @@

				}

				void Framework::DidCloseProductsPopup(ProductsPopupCloseReason reason) const

				{

vng (Migrated from github.com) commented

2024-11-20 10:47:33 +00:00

ProductSettings::Instance().Get()

We can store actual already parsed instance do avoid parsing every time we show PP:

class ProductSettings
{
public:
  ProductSettings()
  {
     lock_guard guard(m_mutex);
     FileReader r(...);
     r.Read(json);
     m_config = Parse(json);
  }
  void Update(std::string const & json)
  {
    lock_guard guard(m_mutex);
    FileWriter w(...);
    w.Write(json);
    m_config = Parse(json);
  }
  ProductConfig Get()
  {
    lock_guard guard(m_mutex);
    return m_config;
  }
};

ProductSettings::Instance().Get() We can store actual already parsed instance do avoid _parsing_ every time we show PP: ``` class ProductSettings { public: ProductSettings() { lock_guard guard(m_mutex); FileReader r(...); r.Read(json); m_config = Parse(json); } void Update(std::string const & json) { lock_guard guard(m_mutex); FileWriter w(...); w.Write(json); m_config = Parse(json); } ProductConfig Get() { lock_guard guard(m_mutex); return m_config; } }; ```

kirylkaveryn reviewed 2024-11-20 16:49:24 +00:00

platform/servers_list.cpp

					
				@ -38,7 +43,13 @@ std::optional<MetaConfig> ParseMetaConfig(std::string const & jsonStr)

				          outMetaConfig.m_settings[key] = valueStr;

				      }

kirylkaveryn commented

2024-11-20 16:49:24 +00:00

Fixed! Thanks

kirylkaveryn reviewed 2024-11-20 16:49:38 +00:00

map/framework.cpp

					
				@ -3316,0 +3361,4 @@

				}

				void Framework::DidCloseProductsPopup(ProductsPopupCloseReason reason) const

				{

kirylkaveryn commented

2024-11-20 16:49:38 +00:00

Fixed! Thanks

kirylkaveryn reviewed 2024-11-20 16:49:48 +00:00

platform/settings.hpp

					
				@ -76,32 +77,8 @@ public:

				  void EnterForeground();

				  void EnterBackground();

kirylkaveryn commented

2024-11-20 16:49:48 +00:00

Fixed! Thanks

vng (Migrated from github.com) reviewed 2024-11-20 17:12:11 +00:00

platform/settings.hpp

					
				@ -76,32 +77,8 @@ public:

				  void EnterForeground();

				  void EnterBackground();

vng (Migrated from github.com) commented

2024-11-20 17:09:01 +00:00

std::string str;
uint64_t time;
if (m_ss.GetValue(m_firstLaunch, str) && FromString(str, time))
  return time;
else  // IDK, return current time ..
  return m_enterForegroundTime;

``` std::string str; uint64_t time; if (m_ss.GetValue(m_firstLaunch, str) && FromString(str, time)) return time; else // IDK, return current time .. return m_enterForegroundTime; ```

vng (Migrated from github.com) reviewed 2024-11-20 17:29:25 +00:00

map/framework.hpp

					
				@ -758,0 +774,4 @@

				private:

				  bool ShouldShowProducts() const;

				  uint32_t GetTimeoutForReason(ProductsPopupCloseReason reason) const;

				  std::string_view ToString(ProductsPopupCloseReason reason) const;

vng (Migrated from github.com) commented

2024-11-20 17:27:00 +00:00

nit: Move just above the DidCloseProductsPopup function.

platform/products.cpp

vng (Migrated from github.com) commented

2024-11-20 17:25:08 +00:00

nit: { from new string

platform/servers_list.cpp

vng (Migrated from github.com) commented

2024-11-20 17:29:19 +00:00

nit: A true constexpr is char const kProductsConfig[] = "productsConfig";

nit: A true constexpr is ```char const kProductsConfig[] = "productsConfig";```

vng (Migrated from github.com) reviewed 2024-11-20 17:30:34 +00:00

platform/products.hpp

					
				@ -0,0 +2,4 @@

				#include <string>

				#include <vector>

				#include <optional>

vng (Migrated from github.com) commented

2024-11-20 17:30:27 +00:00

#include <mutex>

```#include <mutex>```

vng (Migrated from github.com) approved these changes 2024-11-20 18:29:04 +00:00

platform/settings.cpp

					
				@ -425,18 +426,16 @@ void UsageStats::EnterBackground()

				  m_ss.SetValue(m_sessions, ToString(m_sessionsCount));

				}

vng (Migrated from github.com) commented

2024-11-20 18:28:00 +00:00

Sorry, I think that it is a valid situation:

else if (m_enterForegroundTime > 0)
  return m_enterForegroundTime;
else
{
  LOG(LERROR, ("Failed to get first launch time"));
  return TimeSinceEpoch();
}

Sorry, I think that it is a valid situation: ``` else if (m_enterForegroundTime > 0) return m_enterForegroundTime; else { LOG(LERROR, ("Failed to get first launch time")); return TimeSinceEpoch(); } ```

vng (Migrated from github.com) reviewed 2024-11-20 18:35:30 +00:00

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-20 18:35:30 +00:00

Better to declare constants in both cases, e.g:

#ifdef DEBUG
  uint32_t constexpr kFirstLaunchTimeout = 60 * 3; // 3 min because it may takes some time to download the map
#else
  uint32_t constexpr kFirstLaunchTimeout = 60 * 60 * 3; // 3 hours
#endif

Better to declare constants in both cases, e.g: ``` #ifdef DEBUG uint32_t constexpr kFirstLaunchTimeout = 60 * 3; // 3 min because it may takes some time to download the map #else uint32_t constexpr kFirstLaunchTimeout = 60 * 60 * 3; // 3 hours #endif ```

kirylkaveryn reviewed 2024-11-20 18:56:53 +00:00

map/framework.cpp

kirylkaveryn commented

2024-11-20 18:56:53 +00:00

Thx. Fixed!

kirylkaveryn reviewed 2024-11-20 18:56:59 +00:00

platform/settings.cpp

					
				@ -425,18 +426,16 @@ void UsageStats::EnterBackground()

				  m_ss.SetValue(m_sessions, ToString(m_sessionsCount));

				}

kirylkaveryn commented

2024-11-20 18:56:59 +00:00

Thx!

vng (Migrated from github.com) reviewed 2024-11-20 19:17:33 +00:00

vng (Migrated from github.com) left a comment

Did review of Framework::ShouldShowProducts()

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-20 19:13:39 +00:00

I suspect that should be:
uint64_t popupCloseTime = 0;
uint64_t productSelectTime = 0;
What is the reason to make settings::Set(..., 0) ?

- I suspect that should be: uint64_t popupCloseTime = 0; uint64_t productSelectTime = 0; - What is the reason to make settings::Set(..., 0) ?

vng (Migrated from github.com) commented

2024-11-20 19:14:40 +00:00

popupCloseTime == 0 is not needed after valid initialization.

```popupCloseTime == 0``` is not needed after valid initialization.

platform/settings.cpp

					
				@ -23,6 +23,7 @@ using namespace std;

				std::string_view kMeasurementUnits = "Units";

vng (Migrated from github.com) commented

2024-11-20 19:17:00 +00:00

Probably last 2 options are needed only in framework.cpp, no?

kirylkaveryn reviewed 2024-11-21 07:55:24 +00:00

map/framework.cpp

kirylkaveryn commented

2024-11-21 07:55:24 +00:00

popupCloseTime == 0 and productSelectTime == 0 are cases when there is no saved user action (the prompt has never been closed by any reason) and in this case we should always show the promp

kirylkaveryn reviewed 2024-11-21 08:20:48 +00:00

map/framework.cpp

kirylkaveryn commented

2024-11-21 08:20:48 +00:00

I suggest the explicitly set the 0 if there are no such fields in settings

  uint64_t popupCloseTime;
  if (!settings::Get(settings::kPlacePageProductsCloseTime, popupCloseTime))
    popupCloseTime = 0; // popup was never shown
  uint64_t productSelectTime;
  if (!settings::Get(settings::kPlacePageProductsSelectedTime, productSelectTime))
    productSelectTime = 0; // product was never selected

  bool const closePopupTimeoutExpired = popupCloseTime == 0 || popupCloseTime + kLastProductCloseTimeout < now;
  bool const productSelectTimeoutExpired = productSelectTime == 0 || productSelectTime + kLastProductSelectTimeout < now;
  if (closePopupTimeoutExpired && productSelectTimeoutExpired)
    return true;

I suggest the explicitly set the 0 if there are no such fields in settings ```cpp uint64_t popupCloseTime; if (!settings::Get(settings::kPlacePageProductsCloseTime, popupCloseTime)) popupCloseTime = 0; // popup was never shown uint64_t productSelectTime; if (!settings::Get(settings::kPlacePageProductsSelectedTime, productSelectTime)) productSelectTime = 0; // product was never selected bool const closePopupTimeoutExpired = popupCloseTime == 0 || popupCloseTime + kLastProductCloseTimeout < now; bool const productSelectTimeoutExpired = productSelectTime == 0 || productSelectTime + kLastProductSelectTimeout < now; if (closePopupTimeoutExpired && productSelectTimeoutExpired) return true; ```

kirylkaveryn reviewed 2024-11-21 08:23:32 +00:00

platform/settings.cpp

					
				@ -23,6 +23,7 @@ using namespace std;

				std::string_view kMeasurementUnits = "Units";

kirylkaveryn commented

2024-11-21 08:23:32 +00:00

Yes. But I suppose that it is better to store all the setting keys in settings to avoid accidental key duplication and unnecessary string literal usage. It will guarantee key uniqueness

Yes. But I suppose that it is better to store all the setting keys in `settings` to avoid accidental key duplication and unnecessary string literal usage. It will guarantee key uniqueness

vng (Migrated from github.com) reviewed 2024-11-21 11:33:40 +00:00

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-21 11:33:40 +00:00

Yes, and if popupCloseTime == 0, popupCloseTime + kLastProductCloseTimeout < now is true.
One condition is enough here.

Yes, and if ```popupCloseTime == 0```, ```popupCloseTime + kLastProductCloseTimeout < now``` is true. One condition is enough here.

vng (Migrated from github.com) reviewed 2024-11-21 11:34:31 +00:00

platform/settings.cpp

					
				@ -23,6 +23,7 @@ using namespace std;

				std::string_view kMeasurementUnits = "Units";

vng (Migrated from github.com) commented

2024-11-21 11:34:31 +00:00

We already have a dozen keys declared in framework.cpp
From my perspective, the more local scope they have - the better.

We already have a dozen keys declared in framework.cpp From my perspective, the more local scope they have - the better.

vng (Migrated from github.com) approved these changes 2024-11-21 11:51:53 +00:00

vng (Migrated from github.com) reviewed 2024-11-21 14:34:04 +00:00

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-21 14:34:03 +00:00

I think that 2 hours is too much. 30 minutes max, IMO.

I did a quick check, my OM Beta total foreground now is 3570 < 1 hour. I'm developer and active tester :)
Did uninstall and install Beta 2 weeks ago probably.

I think that 2 hours is too much. 30 minutes max, IMO. I did a quick check, my OM Beta total foreground now is 3570 < 1 hour. I'm developer and active tester :) Did uninstall and install Beta 2 weeks ago probably.

vng (Migrated from github.com) reviewed 2024-11-21 14:51:31 +00:00

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-21 14:51:30 +00:00

1 week of my developer-tester metrics:

US_FirstLaunch : 1731609060
US_LastBackground : 1732198335
US_SessionsCount : 105
US_TotalForeground : 3570

1 week of my developer-tester metrics: US_FirstLaunch : 1731609060 US_LastBackground : 1732198335 US_SessionsCount : 105 US_TotalForeground : 3570

kirylkaveryn reviewed 2024-11-21 15:05:39 +00:00

map/framework.cpp

kirylkaveryn commented

2024-11-21 15:05:39 +00:00

Updated to 15 min!

vng (Migrated from github.com) reviewed 2024-11-22 11:48:16 +00:00

map/framework.cpp

					
				@ -3316,0 +3346,4 @@

				  auto const now = base::SecondsSinceEpoch();

				  auto const timeout = GetTimeoutForReason(FromString(productCloseReason));

				  bool const timeoutExpired = popupCloseTime + timeout < now;

vng (Migrated from github.com) commented

2024-11-22 11:48:16 +00:00

Let's take out the function:

bool UsageStats::IsLoyalUser() const
{
#ifdef DEBUG
  uint32_t constexpr kTotalForegroundTimeout = 30;
  uint32_t constexpr kSessionsCount = 3;
#else
  uint32_t constexpr kTotalForegroundTimeout = 60 * 15; // 15 min
  uint32_t constexpr kSessionsCount = 5;
#endif

  return m_sessionsCount >= kMinSessionsCount && m_totalForegroundTime >= kTotalForegroundTimeout;
}

And call it before IsPointCoveredByDownloadedMaps

Let's take out the function: ``` bool UsageStats::IsLoyalUser() const { #ifdef DEBUG uint32_t constexpr kTotalForegroundTimeout = 30; uint32_t constexpr kSessionsCount = 3; #else uint32_t constexpr kTotalForegroundTimeout = 60 * 15; // 15 min uint32_t constexpr kSessionsCount = 5; #endif return m_sessionsCount >= kMinSessionsCount && m_totalForegroundTime >= kTotalForegroundTimeout; } ``` And call it before IsPointCoveredByDownloadedMaps

vng (Migrated from github.com) reviewed 2024-11-22 12:40:52 +00:00

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-22 12:40:51 +00:00

nit: *m_infoGetter

nit: ```*m_infoGetter```

kirylkaveryn reviewed 2024-11-22 12:49:08 +00:00

map/framework.cpp

					
				@ -3316,0 +3346,4 @@

				  auto const now = base::SecondsSinceEpoch();

				  auto const timeout = GetTimeoutForReason(FromString(productCloseReason));

				  bool const timeoutExpired = popupCloseTime + timeout < now;

kirylkaveryn commented

2024-11-22 12:49:08 +00:00

Nice idea! Thanks!

pastk reviewed 2024-11-22 16:43:42 +00:00

map/framework.cpp

pastk commented

2024-11-22 16:43:42 +00:00

The "Remind Later" and close "x" behavior should be different.
Now both lead to a 2 weeks delay till the next popup.

IMO its too long for "Remind Later", maybe a week?

People will choose the "x" button to close the popup if their reason is different from other available options (donate, remind later, already donated): don't want to donate, no money, etc.

There is no point to display the popup again soon to users who e.g. doesn't want to donate, but it could lead to unwanted irritation.

So its better to change the "x" close option to a long delay like it was in the initial version - maybe same as Already Donated 180 days.

The "Remind Later" and close "x" behavior should be different. Now both lead to a 2 weeks delay till the next popup. IMO its too long for "Remind Later", maybe a week? People will choose the "x" button to close the popup if their reason is different from other available options (donate, remind later, already donated): don't want to donate, no money, etc. There is no point to display the popup again soon to users who e.g. doesn't want to donate, but it could lead to unwanted irritation. So its better to change the "x" close option to a long delay like it was in the initial version - maybe same as Already Donated 180 days.

pastk reviewed 2024-11-22 17:03:19 +00:00

platform/settings.cpp

pastk commented

2024-11-22 17:03:19 +00:00

Could we make a longer lead-in delay for brand new users? As proposed in organicmaps/organicmaps#8638

Its better to allow much longer time (e.g. a month) before asking new users to donate. Allow them time to use the app in a real situation (e.g. a trip), learn its functionality and understand/appreciate value it brings.
Also it'd help in cases when users re-install the app often. Alpha/beta testers included?

E.g. if its the first app start then delay the popup for a month?

Could we make a longer lead-in delay for brand new users? As proposed in https://git.omaps.dev/organicmaps/organicmaps/pulls/8638#discussion_r1846961094 > Its better to allow much longer time (e.g. a month) before asking new users to donate. Allow them time to use the app in a real situation (e.g. a trip), learn its functionality and understand/appreciate value it brings. > Also it'd help in cases when users re-install the app often. Alpha/beta testers included? E.g. if its the first app start then delay the popup for a month?

kirylkaveryn reviewed 2024-11-22 17:10:48 +00:00

platform/settings.cpp

kirylkaveryn commented

2024-11-22 17:10:47 +00:00

The 15 in foreground is about 2-3 days of average usage. @vng has made a test somewhere in comments...

pastk reviewed 2024-11-22 17:17:40 +00:00

map/framework.cpp

					
				@ -3316,0 +3378,4 @@

				  uint32_t constexpr kProductSelectTimeout = 20;

				  uint32_t constexpr kRemindMeLaterTimeout = 5;

				  #else

				  uint32_t constexpr kPopupCloseTimeout = 60 * 60 * 24  * 30; // 30 days

pastk commented

2024-11-22 17:07:29 +00:00

why do we need to store the exact donation option a user has chosen?

seems like its not used anywhere

why do we need to store the exact donation option a user has chosen? seems like its not used anywhere

pastk reviewed 2024-11-22 17:22:14 +00:00

platform/settings.cpp

pastk commented

2024-11-22 17:22:13 +00:00

15 minutes is very little in terms of getting to know the app.

In terms of the foreground time it should be several hours.

15 minutes is very little in terms of getting to know the app. In terms of the foreground time it should be several hours.

pastk reviewed 2024-11-22 18:38:46 +00:00

map/framework.cpp

					
				@ -3316,0 +3378,4 @@

				  uint32_t constexpr kProductSelectTimeout = 20;

				  uint32_t constexpr kRemindMeLaterTimeout = 5;

				  #else

				  uint32_t constexpr kPopupCloseTimeout = 60 * 60 * 24  * 30; // 30 days

pastk commented

2024-11-22 18:38:46 +00:00

btw I assume a user could change the donation option (amount, recurring or one-time) after the button press? i.e. while on the stripe page already?

vng (Migrated from github.com) reviewed 2024-11-22 23:21:28 +00:00

platform/settings.cpp

vng (Migrated from github.com) commented

2024-11-22 23:21:28 +00:00

organicmaps/organicmaps#9695

With this branch organicmaps/organicmaps#9710
you can get US_* log entries (when the app starts) to check your usage. Several hours in foreground is way too much.

https://git.omaps.dev/organicmaps/organicmaps/pulls/9695#discussion_r1852288314 With this branch https://git.omaps.dev/organicmaps/organicmaps/pulls/9710 you can get US_* log entries (when the app starts) to check your usage. Several hours in foreground is way too much.

vng (Migrated from github.com) reviewed 2024-11-22 23:23:31 +00:00

map/framework.cpp

					
				@ -3316,0 +3378,4 @@

				  uint32_t constexpr kProductSelectTimeout = 20;

				  uint32_t constexpr kRemindMeLaterTimeout = 5;

				  #else

				  uint32_t constexpr kPopupCloseTimeout = 60 * 60 * 24  * 30; // 30 days

vng (Migrated from github.com) commented

2024-11-22 23:23:31 +00:00

Yes, he can do anything after the donate page, but we save what button prompted him to go.

pastk reviewed 2024-11-23 11:55:52 +00:00

platform/settings.cpp

pastk commented

2024-11-23 11:55:52 +00:00

The same 15min foreground time could differ wildly in terms of the exploration of app's features.

E.g. you're an experienced user and tend to be very efficient when you beta-test or use the app, but for a new user 15min could be just enough to download some maps and browse around a little. Or its just 15min of a drive (using the navigation feature) and not much else.

I think 15min won't be enough even for a weekend trip for most new users.
Let's make it an hour at least? (could translate to somewhere between a week and a month of "real time", which looks reasonable)

It applies for new users only, of course.
For existing users there is no need to wait 15min even, e.g. make it 3min (as we're sure existing users have had the app installed at least 2 weeks before according to our update cycle).

The same 15min foreground time could differ wildly in terms of the exploration of app's features. E.g. you're an experienced user and tend to be very efficient when you beta-test or use the app, but for a new user 15min could be just enough to download some maps and browse around a little. Or its just 15min of a drive (using the navigation feature) and not much else. I think 15min won't be enough even for a weekend trip for most new users. Let's make it an hour at least? (could translate to somewhere between a week and a month of "real time", which looks reasonable) It applies for new users only, of course. For existing users there is no need to wait 15min even, e.g. make it 3min (as we're sure existing users have had the app installed at least 2 weeks before according to our update cycle).

rtsisyk approved these changes 2024-11-24 16:13:36 +00:00

rtsisyk left a comment

Looks great!

rtsisyk reviewed 2024-11-24 16:15:20 +00:00

map/framework.cpp

rtsisyk commented

2024-11-24 16:15:20 +00:00

Please set Later to 3 days.

pastk reviewed 2024-11-25 10:22:44 +00:00

map/framework.cpp

pastk commented

2024-11-25 10:22:44 +00:00

The opinion poll in OM dev chats showed that weighted average preference is 2.4 months ((0.5 * 3 + 1 * 3 + 3 * 4 + 6 * 2) / 12 = 2.38) (based on votes of 12 developers).

I guess we can round it to 2.5 months / 75 days?

The opinion poll in OM dev chats showed that weighted average preference is 2.4 months (`(0.5 * 3 + 1 * 3 + 3 * 4 + 6 * 2) / 12 = 2.38`) (based on votes of 12 developers). I guess we can round it to 2.5 months / 75 days?

vng (Migrated from github.com) reviewed 2024-11-25 11:43:58 +00:00

vng (Migrated from github.com) left a comment

I propose:

increase to 30 min foreground (2-3 hours in our reality means never)
increase close to 1 month
add additional PPOpenCount and show donate when: ++PPOpenCount % 3 == 0.

I propose: - increase to 30 min foreground (2-3 hours in our reality means never) - increase close to 1 month - add additional PPOpenCount and show donate when: ++PPOpenCount % 3 == 0.

map/framework.cpp

vng (Migrated from github.com) commented

2024-11-25 11:40:55 +00:00

Strange logic now here.
We should save kPlacePageProductsPopupCloseTime (ok now) and last ProductsPopupCloseReason instead of kPlacePageProductRemindMeLaterTime or kPlacePageProductSelectionTime.
Then according to the last saved ProductsPopupCloseReason we select a timeout to check in ShouldShowProducts.

Strange logic now here. We should save kPlacePageProductsPopupCloseTime (ok now) and last ProductsPopupCloseReason instead of kPlacePageProductRemindMeLaterTime or kPlacePageProductSelectionTime. Then according to the last saved ProductsPopupCloseReason we select a timeout to check in ShouldShowProducts.

rtsisyk reviewed 2024-11-25 11:57:03 +00:00

map/framework.cpp

rtsisyk commented

2024-11-25 11:57:03 +00:00

Thanks for running the poll. There are no illusions that this new donation reminder may be bothersome. Reminders are meant to be a somehow annoying - that is what they are for. Ask people how often they want to be annoyed, and the option to not add this reminder at all would easily win any poll by a large margin.

The proposed UI is fairly moderate and far from being as disruptive as a paywall. This particular constant in the code is for "X" (Close) button that plays "Remind me later" function. 2.5 months, or 75 days, is an eternity in terms of an app's lifecycle. Most people probably do not even remember the app they used after just a few months. Two weeks is probably the maximum reasonable value here to make this reminder functional. People who donated can press "I've donated already" button to hide the message for 6 months.

Thanks for running the poll. There are no illusions that this new donation reminder may be bothersome. Reminders are meant to be a somehow annoying - that is what they are for. Ask people how often they want to be annoyed, and the option to not add this reminder at all would easily win any poll by a large margin. The proposed UI is fairly moderate and far from being as disruptive as a paywall. This particular constant in the code is for "X" (Close) button that plays "Remind me later" function. 2.5 months, or 75 days, is an eternity in terms of an app's lifecycle. Most people probably do not even remember the app they used after just a few months. Two weeks is probably the maximum reasonable value here to make this reminder functional. People who donated can press "I've donated already" button to hide the message for 6 months.

pastk reviewed 2024-11-25 12:09:59 +00:00

map/framework.cpp

pastk commented

2024-11-25 12:09:59 +00:00

This particular constant in the code is for "X" (Close) button that plays "Remind me later" function.

Did you forget there is a separate "Remind Later" button which is set to 3 days (as per your own suggestion organicmaps/organicmaps#9695) ? :)

The "X" (close) button is for other reasons, e.g. "Don't want to donate".

But please let's not repeat ourselves with the same arguments.

Developers have different opinions on this topic and we couldn't come to an agreement, that's why we had a poll.

> This particular constant in the code is for "X" (Close) button that plays "Remind me later" function. Did you forget there is a separate "Remind Later" button which is set to 3 days (as per your own suggestion https://git.omaps.dev/organicmaps/organicmaps/pulls/9695#discussion_r1855484906) ? :) The "X" (close) button is for other reasons, e.g. "Don't want to donate". But please let's not repeat ourselves with the same arguments. Developers have different opinions on this topic and we couldn't come to an agreement, that's why we had a poll.