Skip to content

Conversation

@grabs
Copy link
Contributor

@grabs grabs commented Sep 29, 2020

Hi,
I added a simple multi lang support for this plugin.
Since the loleaflet.html accepts a lang parameter. So we can use the current language to set the interface language of the collabora instance.
Collabora does not accept all lang strings. So I use a constant with accepted strings. Maybe later we can change this into a setting.
Collabora need strings like en or en-us (note the hyphen). Strings with a wrong second part are also possible which leads to a fallback of the first part. The string de_comm which does not exist in collabora would result in de.
A not accepted first part, e.g. yy_abc will result in an empty page. To prevent this I check the first part of the current language and compare it with the accepted strings list.
Best regards
Andreas

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2020

Codecov Report

Merging #24 into master will decrease coverage by 0.48%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #24      +/-   ##
============================================
- Coverage     14.02%   13.53%   -0.49%     
- Complexity      137      142       +5     
============================================
  Files            17       17              
  Lines           699      724      +25     
============================================
  Hits             98       98              
- Misses          601      626      +25     
Impacted Files Coverage Δ Complexity Δ
moodle/mod/collabora/classes/collabora.php 33.17% <0.00%> (-4.33%) 64.00% <0.00%> (+5.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc22536...7032a63. Read the comment docs.

@mwuttke
Copy link

mwuttke commented Oct 1, 2020

Hello Ashod,

you have made a lot of code changes. Thanks a lot of your work!

Do you have tested all these settings with moodle group and/or grouping mode too?

@grabs please could you review this patch?

Michael

@mwuttke
Copy link

mwuttke commented Oct 1, 2020

Hello Andreas,

I've tested your PR and it works as expected. Thanks for your work!

@Ashod: please could you review this patch?

Michael

Copy link

@Ashod Ashod left a comment

Choose a reason for hiding this comment

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

Looks good, one minor improvement suggested.

}

// If we got here we check the system language as first fallback.
$systemlang = isset($CFG->lang) ?: 'en';
Copy link

Choose a reason for hiding this comment

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

'en' -> self::FALLBACK_LANG

Seems cleaner to avoid hard-coded value when we have a named const.

@Ashod
Copy link

Ashod commented Oct 3, 2020

Hello Ashod,

you have made a lot of code changes. Thanks a lot of your work!

Do you have tested all these settings with moodle moodle group and/or gouping mode too?

Hi @moodlebeuth ,

I suspect the above comment was meant for one of my PRs, Michael?
If so, which one? And what is grouping mode?

Thanks!

@grabs grabs merged commit bc28542 into learnweb:master Oct 3, 2020
mwuttke pushed a commit that referenced this pull request Oct 4, 2020
@mwuttke
Copy link

mwuttke commented Oct 4, 2020

Hi @Ashod: I am sos orry, my fault. I meant #21.

@grabs grabs deleted the gedv_feature_langsupport branch November 26, 2020 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants