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

Display app names in user's language when upgrading. #6025

Closed
wants to merge 3 commits into from

Conversation

tcitworld
Copy link
Member

Fixes #6024

Before:
selection_092

After:
selection_093

@codecov
Copy link

codecov bot commented Aug 8, 2017

Codecov Report

Merging #6025 into master will decrease coverage by 16.03%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #6025       +/-   ##
=============================================
- Coverage     53.06%   37.02%   -16.04%     
- Complexity    22553    22769      +216     
=============================================
  Files          1414     1405        -9     
  Lines         87745    88171      +426     
  Branches       1340     1327       -13     
=============================================
- Hits          46560    32648    -13912     
- Misses        41185    55523    +14338
Impacted Files Coverage Δ Complexity Δ
lib/private/App/AppManager.php 65.44% <0%> (-30.15%) 47 <8> (ø)
lib/base.php 2.89% <0%> (ø) 173 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Activity/Filter/Todo.php 0% <0%> (-100%) 7% <0%> (ø)
apps/sharebymail/lib/Settings.php 0% <0%> (-100%) 3% <0%> (ø)
apps/dav/lib/Files/Sharing/FilesDropPlugin.php 0% <0%> (-100%) 6% <0%> (ø)
...Check/Iterator/ExcludeFileByNameFilterIterator.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/Files/SimpleFS/SimpleFolder.php 0% <0%> (-100%) 11% <0%> (ø)
lib/public/RichObjectStrings/Definitions.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/Template/ResourceNotFoundException.php 0% <0%> (-100%) 2% <0%> (ø)
apps/dav/lib/CalDAV/Plugin.php 0% <0%> (-100%) 2% <0%> (ø)
... and 616 more

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@LukasReschke
Copy link
Member

Rebased to retrigger tests.

@MorrisJobke
Copy link
Member

There was 1 failure:

1) Test\Authentication\TwoFactorAuth\ManagerTest::testGetBackupProvider

Failed asserting that null is identical to an object of class "Mock_IProvider_8d699353".

/drone/src/github.com/nextcloud/server/pull/6025/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php:218

@rullzer
Copy link
Member

rullzer commented Sep 4, 2017

failing tests

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 4, 2017
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
Copy link
Member Author

So I don't know why but it seems phpunit's returnValueMap() was causing trouble with optional arguments.

@tcitworld tcitworld added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 24, 2017
@nickvergessen
Copy link
Member

You have to specify optional arguments all the time in the map

@tcitworld
Copy link
Member Author

You have to specify optional arguments all the time in the map

I tried that too, but it didn't work. :'(

@MorrisJobke
Copy link
Member

I still vote for extracting this in the l10n process and let transifex handle this. Then we should read the app name and description for the l10n files instead of hacking this into the info.xml.

@tcitworld
Copy link
Member Author

@MorrisJobke I'm not sure to understand your comment. 😕

@MorrisJobke
Copy link
Member

@MorrisJobke I'm not sure to understand your comment. 😕

Currently translations of an app are handled via our l10n/ folder holding all the translated files. This is also the case for the app name. The app name is already extracted from info.xml and put on transifex. That means that our UI (apps management, app navigation) can use the translated app name without any further requirements.

The info.xml somehow still has this lang="fr" attribute build in, that is nowhere used (beside in 2 apps) and the same for the description. It would just make sense to not maintain those translations within the info.xml, but also take them, put them on transifex and put the translated terms into the l10n folder like all other translations of an app.

@tcitworld
Copy link
Member Author

OK, so this would mean :

  • extract the description text just like the app name and put it into transifex
  • tweak the nextcloud apps page and the app store to display name and description according to contents of l10n and not just info.xml
  • Explain to app maintainers who don't use transifex for their app that they need to put the name and description strings in the l10n files.

@nickvergessen
Copy link
Member

tweak the nextcloud apps page and the app store to display name and description according to contents of l10n and not just info.xml

Well it's easier, we "just" do \OC::$server->getL10N($appId)->t($appName) and \OC::$server->getL10N($appId)->t($data['description'])

@MorrisJobke
Copy link
Member

Explain to app maintainers who don't use transifex for their app that they need to put the name and description strings in the l10n files.

They either don't translate or reach out to us to get the transifex integration. So I don't see this as a problem.

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 13, 144, Nextcloud 14 Nov 15, 2017
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 6, 2018
@MorrisJobke
Copy link
Member

They either don't translate or reach out to us to get the transifex integration. So I don't see this as a problem.

Let's get this done: nextcloud/docker-ci#93

Then it is properly translatable in transifex and we don't need to keep this in sync manually for all the apps.

@MorrisJobke MorrisJobke deleted the fix-#6024 branch May 23, 2018 16:25
@MorrisJobke MorrisJobke removed this from the Nextcloud 14 milestone May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants