#6307 fix page title not changed

[burned42]

Fixes (on master) #6307
With this changes the page title gets set when switching to file list and tag list.

[LukasReschke]

Thanks a lot for your Pull Request! 🚀

Unfortunately, this seems to make the JSUnit tests fail as per https://drone.nextcloud.com/nextcloud/server/2180/241. Would you mind looking into this?

Also cc @danxuliu as he may be able to help here :)

[codecov]

Codecov Report

Merging #6869 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #6869 +/- ##
============================================
- Coverage 52.82% 52.82% -0.01% 
Complexity 22802 22802 ============================================
Files 1442 1442 Lines 88562 88563 +1 Branches 1349 1349 ============================================
- Hits 46787 46783 -4 - Misses 41775 41780 +5

Impacted Files	Coverage Δ	Complexity Δ
apps/systemtags/js/systemtagsfilelist.js	73.27% <100%> (+0.23%)	0 <0> (ø)	⬇️
core/js/js.js	61.27% <0%> (-0.56%)	0% <0%> (ø)
lib/private/Server.php	83.47% <0%> (-0.13%)	124% <0%> (ø)
lib/private/Security/CertificateManager.php	92.07% <0%> (+0.99%)	39% <0%> (ø)	⬇️

[burned42]

@LukasReschke Sorry, I did only read about PHPUnit tests in the CONTRIBUTING.md. I will check the errors and see if I can fix them.

[burned42]

@LukasReschke It seems like there are still some failing tests but I'm not sure if my commits are the cause. The JSUnit test is not failing anymore.

[LukasReschke]

The failing tests are indeed unrelated. Thanks a lot for your pull request!

@icewind1991@danxuliu Opinions here? :)

[danxuliu]

Thanks for your contribution, @burned42 :-D

These changes make the base file list and the system tags file list follow the same approach used with the file lists for favorites, recents and so on, and consistency is good ;-)

The only problem is that calling _setCurrentDir in reload is redundant in some cases in which _setCurrentDir is called just before reload (for example, in changeDirectory), and probably the other file lists should have not been calling _setCurrentDir in reload in the first place; the code of the file lists needs some review and clean up anyway, so I am OK with merging this now and taking care of those things later ;-)

Note, however, that #6307 is not fixed by this pull request, because that issue is filled against Nextcloud 12, and these changes will be merged into master and thus Nextcloud 13. @burned42, once these changes are merged, could you backport this to the stable12 branch too? :-)

[burned42]

@danxuliu Since I am unfamiliar with the codebase I checked the other modules/files and tried to make it consistent above these. I noticed the redundant call but didn't want to start big refactoring since, as I said, I am not really familiar with the codebase.

Sure, I could try to backport these changes. Is there a best practice on how to do that? Should I just checkout stable12 and try to apply the same changes?

[danxuliu]

@burned42

Since I am unfamiliar with the codebase I checked the other modules/files and tried to make it consistent above these. I noticed the redundant call but didn't want to start big refactoring since, as I said, I am not really familiar with the codebase.

Of course! Please do not see my comment as a critic to your changes; it was just a comment about the general state of the FileList code :-)

Sure, I could try to backport these changes. Is there a best practice on how to do that? Should I just checkout stable12 and try to apply the same changes?

Yes, basically that is it.

What I usually do is, on the branch that I want to backport, create a new branch with git checkout -b stable12-{original-PR-number}-{original-PR-branch-name} (but that is just how I name it; others use different name schemes) and then rebase the new branch onto stable12 using git rebase --onto stable12 {commit-id-where-the-branch-to-be-backported-diverged-from-master} stable12-{original-PR-number}-{original-PR-branch-name}.

And then, of course, test to ensure that everything works as expected ;-)

By the way, before backporting, may I ask you to fold the third and fourth commit into the first one (for example, with git rebase -i HEAD~4, replacing pick with fixup in the third and fourth commits, and then moving the second commit to the end) to get a cleaner commit history? Sorry, I am a professional nitpicker :-P

Thank you! :-)

[burned42]

@danxuliu just to clarify, 'branch that I want to backport' would be my current branch 6307-fix_page_title_not_changed, correct?

Regarding your other request, by 'before backporting' do you mean I should do the rebase now or after this pull request is merged?

[danxuliu]

@burned42

to clarify, 'branch that I want to backport' would be my current branch 6307-fix_page_title_not_changed, correct?

Correct.

Regarding your other request, by 'before backporting' do you mean I should do the rebase now or after this pull request is merged?

If possible please do it before this pull request is merged so we have a cleaner commit history also in master :-) You will have to force push the 6307-fix_page_title_not_changed branch to discard the previous commits already on the GitHub server.

[burned42]

@danxuliu Rebased as suggested and force pushed the changes.

[danxuliu]

Great! Thank you very much! :-)

[burned42]

@danxuliu Thank you for your help, also @LukasReschke. I already followed your other request and after testing the changes with stable12 I will create a pull request for that, too.