[FIX] Option "Copy here" in spaces list after rotating device

[mykh-hailo]

Related Issues

App: #4718

• Add changelog files for the fixed issues in folder changelog/unreleased. More info here
• Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

[mykh-hailo]

@joragua Can you check this PR please?

[joragua]

Somme comments here @mykh-hailo! 🙌🏻

NOTE: We usually create the branches using underscores between words. In this case, the name of the branch would be: fix/copy_button_visibility_on_rotation (just for you to know) Do not remove the branch but just keep it in mind for future PRs 😄

[mykh-hailo]

Thank you for your comment @joragua
I will fix for your comments asap.
Thank you.

[mykh-hailo]

@joragua I implemented much easier solution for this issue.
Please check and let me know if you have any comments.
Thank you.

[joragua]

@mykh-hailo Just one more comment and it's ready to go!

[mykh-hailo]

@joragua I made a quick fix on your comment.
I'd appreciate it if you check this again.
Thank you.

[joragua]

Good job! 💯 Let's move it to QA

[jesmrec]

@mykh-hailo Unfortunately the fix it's no valid. It fixes the problem with the Copy but breaks the Move.

Check the following steps out:

1. In the list of files, select any item to Move -> List of files displayed (not spaces list, since it's not allowed to move items through spaces), with Move here and Cancel options available.
2. Rotate the device

Move here is gone (it shouldn't) and only Cancel is available.

It's verified that the current master branch shows the correct behaviour for the Move, therefore the defect comes from this PR.

Could you take a look? anything you need, don't hesitate to ask us!

[joragua]

After some testing, I've noticed that his bug (#4441) is also reproducible in the folder picker. It’s likely that both issues could be fixed with a single solution. But we will need to check it in QA... 🤔

Here's the other PR (#4467) related to this problem, if it helps @mykh-hailo 🙌🏻

[mykh-hailo]

@joragua as far as I checked the code again based on Jesmerc's comment, I think my first approach to call a function to change visibility of that function onResume would be a solution.
What do you think?

[jesmrec]

Then, we have two different approaches:

1. Modifying the Manifest file adding a property like android:configChanges="orientation|screenSize" , suggested by @joragua

2. Initial solution suggested by @mykh-hailo

First one intends to fix two problems in one ✅ , but, Manifest stuff is delicated and will need extra testing, but it is the cleanest solution IMO (if nothing is broken, for sure).

Second one seems to be totally safe ✅ , but more code to achieve the same target.

@mykh-hailo @joragua do you have any extra input? pros and cons of every solution? let's make it constructive 😄

[jesmrec]

OTOH, just as a comment, @mykh-hailo we use to write branch names with underscores (check GUIDELINES), so, the repo will be tidy and uniform around ;) . Take it in account for future PRs, thanks!!

[joragua]

This is my opinion:

Approaches	Pros	Cons
android:configChanges="orientation|screenSize"	Could fix two different bugs at the same time related to the folder picker. Cleaner solution in terms of logic, fewer lines of code, and better readability.	Requires more testing to ensure it doesn’t break the folder picker since Manifest is delicated as mentioned
@mykh-hailo solution	Could fix the bug related to the "Copy here" option.	Another fix is still needed for the filter in the folder picker (the same as the other approach? redundant code?). Results in more code and more lines.

[mykh-hailo]

@joragua as far as I checked the code the filtering issue is caused because my branch was not up-to-date.

[joragua]

Hmm... 🤔 I don't think so, since I can reproduce the filter bug on the master branch. Can you confirm this @jesmrec?

[jesmrec]

The problem about filtering that @joragua reported is reproducible in master , and also visible in the stable version. That means, it's not a matter of your branch @mykh-hailo.

That also means, if you'd wish you can fix it together with the initial bug (both problems with one fix would be cool), but, as a regression problem, it could be addressed separately. Up 2 you ;)

[mykh-hailo]

@jesmrec Let me fix the both issues together.

[mykh-hailo]

@jesmrec Can you check my updates again?
one fix for 2 issues.