search

cgeo / cgeo

c:geo - The powerful Android geocaching app.
www.cgeo.org
Apache License 2.0
1.39k stars 565 forks source link

Next feature release #8615

Closed moving-bits closed 4 years ago

moving-bits commented 4 years ago

Let's open this issue to discuss which things should go into the next feature release, and which should stay in the queue for now.

moving-bits commented 4 years ago

Looking at the current PR queue the following things should/can go into the next feature release IMHO:

The other open PR I would like to postpone for now:

For #8611 it depends, see below.

Also, I would like to finish my work on the "fast scroll" behavior. Guessing from the feedback it seems to work reasonably well now for the cache list, so I can start extracting it to a reusable feature to be applied to other lists in c:geo which have fastscroll enabled.

One thing that we should discuss is how to proceed with "online maps onboarding":

We should decide whether we want to keep the implementation of #8183 or not:

My proposal would be:

This would remove the ability to open locally stored .map files, but would offer c:geo much less frequently when tapping on local files, avoiding the effects described in #8467.

@Lineflyer What do you think?

Lineflyer commented 4 years ago

Thanks @moving-bits for the good summary.

Regarding the maps onboarding feature: I did meanwhile understand the reason of the "cgeo file manager", however I can still not believe (while I have no technical background to judge) that there is no way to limit this intent somehow. Newest finding: If I look into my contacts it offers to send a message to my contact using c:geo file manager. Let me say: I would prefer, that this is removed just to avoid the clutter on users device and the mass amount of questions and support cases I do expect based on that. (Will comment in more detail on #8467). That being said: I would also vote for the second approach, but as I always prefer time for testing I would additionally suggest to remove the first approach (or at least the parts involving this c:geo file manager) and try the secons approach only after merging branches.

For the list of PRs above I agree for most, only regarding #8589 I am a bit undecided as I did not yet understand if this is just a bugfix or also giving up some existing parsings.

moving-bits commented 4 years ago

Ok, let's do it that way. I merged #8612 and prepared two new PR:

PR #8611 is postponed for now, as it's needed only for the new "map downloader" currently (and might be used for #8603 as well).

As soon as we have branched I can merge the new "map downloader" to master so that you have a chance to test it. Then we can still decide whether to integrate it into the upcoming release or into a later release.

Lineflyer commented 4 years ago

As soon as we have branched I can merge the new "map downloader" to master so that you have a chance to test it. Then we can still decide whether to integrate it into the upcoming release or into a later release

Sound like a good plan! :)

moving-bits commented 4 years ago

@Lineflyer: Anything that needs to be done for the next release regarding #8517?

moving-bits commented 4 years ago

@Lineflyer: How about https://github.com/cgeo/cgeo/issues/8477#issuecomment-650517642?

Lineflyer commented 4 years ago

IMHO the #8477 comment was solved or not. I will test a fresh installation and compare, because now I already played around with the settings.

Lineflyer commented 4 years ago

@Lineflyer: Anything that needs to be done for the next release regarding #8517?

I will doublechek as time permits. Just merged the german translations and look over it next days.

Lets keep the nightlies/master stable next days to see if its working smooth. Then we take action.

moving-bits commented 4 years ago

IMHO the #8477 comment was solved or not. I will test a fresh installation and compare, because now I already played around with the settings.

I have not changed the way "history track line" gets drawn yet - it's still a "broad" line (standard: grey) with a smaller white line on top. As this is the only line drawn in this way this might be a reason to change it to the way all other lines are drawn - just as a plain line. OTOH this would give a different visual effect.

moving-bits commented 4 years ago

@Lineflyer: Anything that needs to be done for the next release regarding #8517?

I will doublechek as time permits. Just merged the german translations and look over it next days.

Thanks.

Lets keep the nightlies/master stable next days to see if its working smooth. Then we take action.

Yes. I have merged the "fastscroll" and "disable file manager" PR already, so other than the two issues mentioned above (#8517 / https://github.com/cgeo/cgeo/issues/8477#issuecomment-650517642) there's currently nothing new planned for master branch from my side.

(BTW: #8477 itself is resolved, just the linked comment is TBD.)

Lineflyer commented 4 years ago

@moving-bits Do you have an overview whether there are any regression issues in our lists (meaning regressions compared to current release)? I will also do a scan of our issues next days.

Lineflyer commented 4 years ago

Oh yes: #8457

moving-bits commented 4 years ago

Oh yes: #8457

This should not happen for now on devices running up to Android 10, as I'd enabled the official mitigation for it (android:requestLegacyExternalStorage="true" in AndroidManifest.xml).

But it will not work on the upcoming Android 11 devices, as Android enforces the new rules then. For that we'll have to rewrite our storage framework.

Lineflyer commented 4 years ago

This should not happen for now on devices running up to Android 10, as I'd enabled the official mitigation for it (android:requestLegacyExternalStorage="true" in AndroidManifest.xml).

I missed that, so I will give it another test run.

Lineflyer commented 4 years ago

Seems we look rather good now on our way to a new beta version. Only #8597 is left over to be reviewed and merged if OK.

Based on my personal timing I would suggest two different options to decide upon:

  1. Keep master stable for a week (suggest until 24.07. as I am on vacation from 17.07. - 24.07. ;) ). If no big problems arise, merge branches and continue publish a beta version
  2. Merge branches before 17.07. and publish a beta. This will allow collecting feedback a week earlier and not block further process on master

I would prefer option 2, the only downside is, that I might probably unable to help and answer support mail if there is any serious problem on beta. But as I do not expect that, I would still prefer the faster approach.

moving-bits commented 4 years ago

Both options would be fine for me, so go ahead with option 2 if you like.

Can you give me a short feedback on https://github.com/cgeo/cgeo/issues/8477#issuecomment-650517642? That's about if we want to change history track line to a regular non-shadowed line like all the other lines are. IMHO now would be a good timing for that with all that formatting changes for the map lines. If ok from your side I would prepare this as small PR for upcoming feature release. - Except if you say better not to change the shadowing.

Lineflyer commented 4 years ago

For #8457 I tested the workaround on Android 10 and its working.

So @cgeo/team please double check if there is anything to do on master before merging branches (potentially tomorrow). What about #8597?

moving-bits commented 4 years ago

8597 is merged meanwhile.

I'm unsure about #8646:

No idea for #8632 yet, but for a beta we may continue for now.

8647 should be included - avoids painting OSM history track line in a "dotted style".

Lineflyer commented 4 years ago

I left yesterday for vacation. This means I could still merge branchs, but beta version only next week.

Just tell me, when you think everything is in.

moving-bits commented 4 years ago

Without further information I cannot reproduce #8632 nor #8646, so I'm a bit lost. Are those two affecting other people as well, anything on support? Shall I implement mitigations (null checks avoiding the NPE)?

Other than that nothing urgent left on my radar currently (but I may have missed something), so IMHO branches could be merged any time now.

Lineflyer commented 4 years ago

Ok, will do it and publish beta either after vacation or if I find time and technical possibility before. Will report back here.

moving-bits commented 4 years ago

AFAIC we seem to have a pretty stable version now, after #8663 and #8664 having been fixed. Only #8661 and #8670 left on my mind, but both non-critical, and both could be added during a beta phase, IMHO.

Lineflyer commented 4 years ago

I will try to look on it this weekend when back from vacation.

bekuno commented 4 years ago

I have to test the reverts for #7998. I hope that it is finished tomorrow.

Lineflyer commented 4 years ago

OK, so lets wait for:

moving-bits commented 4 years ago

I'm receiving an NPE in Routesort view when loading has not yet finished. Will create issue + PR for that.

moving-bits commented 4 years ago

@Lineflyer I've merged the open issues I'd been working on for master branch (including the fix for the NPE mentioned above), updated changelog accordingly, and added a warning info about Android 11 (due to #8457). From my side "ready to go" except maybe reverting #7998.

Lineflyer commented 4 years ago

I've merged the open issues I'd been working on for master branch (including the fix for the NPE mentioned above), updated changelog accordingly, and added a warning info about Android 11 (due to #8457).

Perfect. thanks.

From my side "ready to go" except maybe reverting #7998.

OK, I will merge branches now. @bekuno For reverting #7998 please submit PR towards release.

Lineflyer commented 4 years ago

Be aware: Branches have been merged

We do now have: release = Currently equals master and base for upcoming beta version, any fixes for regressions should be merged here until release is ready master = Free for new developments cgeo-legacy= Contains status of previous release for reusing as a legacy version parallel to next release

I will try to build, test and publish beta tonight or tomorrow. If possible on Google Play I will also try to bring the legacy version into the beta test.

Lineflyer commented 4 years ago

@moving-bits While smoke testing the possible RC/beta version I stumbled upon an empty changelog page in About c:geo. While merging branches I exchanged the changelog content between changelog_master and changelog_release and hopefully did everything right. The content of changelog_release was previously appended below the changelog_master on the "Changes" tab in About c:geo. Can you please crosscheck whether I did something wrong?

moving-bits commented 4 years ago

Thanks, @Lineflyer, for merging the branches and starting to prepare beta and legacy versions. I will then start merging the PR currently being on hold into master over the next days, to get the queue freed up.

moving-bits commented 4 years ago

While smoke testing the possible RC/beta version I stumbled upon an empty changelog page in About c:geo. While merging branches I exchanged the changelog content between changelog_master and changelog_release and hopefully did everything right. The content of changelog_release was previously appended below the changelog_master on the "Changes" tab in About c:geo. Can you please crosscheck whether I did something wrong?

I'll look into it.

moving-bits commented 4 years ago

While smoke testing the possible RC/beta version I stumbled upon an empty changelog page in About c:geo. While merging branches I exchanged the changelog content between changelog_master and changelog_release and hopefully did everything right. The content of changelog_release was previously appended below the changelog_master on the "Changes" tab in About c:geo. Can you please crosscheck whether I did something wrong?

I'll look into it.

Does work here for both master and legacy branch. (Tested with emulator, API 29)

Lineflyer commented 4 years ago

Release branch is the problem. I tested on an Android 5.0.1 device.

moving-bits commented 4 years ago

Release branch is the problem. I tested on an Android 5.0.1 device.

Ok, I think I've found the culprit: The \n in the middle of the warning message regarding Android 11 confuses Android 5 (although it works on API 29). I'll replace it by " - " (on release branch).

Lineflyer commented 4 years ago

Ok, I think I've found the culprit: The \n in the middle of the warning message regarding Android 11 confuses Android 5 (although it works on API 29). I'll replace it by " - " (on release branch).

Did you already do that? Afterwards I would push the beta versions.

moving-bits commented 4 years ago

Yes, had changed this yesterday on release branch.

Lineflyer commented 4 years ago

OK, I uploaded a new multiple APK beta version: 2020.07.26-RC / 2020.07.24-RC-legacy

It consists of our normal RC from release branch and the legacy version from cgeo-legacy branch. Beta users on API 16-20 should receive the legacy version, API 21+ should receive the normal RC.

I will update versions on status.cgeo.org in a few minutes so that we can see user counts.

Lineflyer commented 4 years ago

I just triggered some proofreadings on crowdin to get more translations in. Please kindly wait with further updates to master until #8677 is tested and merged. Then I will again fast-forward release to the head of master to have these translations included in the upcoming release.

Lineflyer commented 4 years ago

I just triggered some proofreadings on crowdin to get more translations in. Please kindly wait with further updates to master until #8677 is tested and merged. Then I will again fast-forward release to the head of master to have these translations included in the upcoming release.

DONE!

moving-bits commented 4 years ago

Next thing I would like to merge is #8611, as this is the base for two other PR, but CI constantly fails with an error message I do not know the reason for, see my note in #8611. (Something like "no tests found" - looks to me like a problem with the CI.)

Any ideas?

moving-bits commented 4 years ago

@Lineflyer Have you had a look at #8643 from qs & support perspective? (I'm not that deep into proceedings of personal note replication.) If ok, we can merge, but we need to squash the PR first.

Lineflyer commented 4 years ago

Have you had a look at #8643 from qs & support perspective? (I'm not that deep into proceedings of personal note replication.) If ok, we can merge, but we need to squash the PR first.

Well, for sure this problem should be solved. But I am not sure, whether this PR fixes it, as according to PR description it handles merging between provider and local, whereas my experience shows, that the duplication happens while the cache is saved and without any server interaction. But lets test it, if the code looks good to you. We do now have most possible time on master.

Lineflyer commented 4 years ago

Next thing I would like to merge is #8611,

I guess the CI failure is somehow related to your PR as it runs smoothly for other PRs.

Lineflyer commented 4 years ago

Ready to go:

fm-sys commented 4 years ago

Let's close this, since this old release is finished and we are currently preparing a new one...