Follow-up for #2394883-54: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage.

Problem/Motivation

There are currently quite a few places in Drupal Core that generate lists of languages for settings -- some of which are real languages, like "English", "Spanish"; some of which are "derived" languages, like "The detected/selected language of the current page for purposes of UI text".

These lists are generated from different code in different places, and are unfortunately currently not consistent with each other. This can lead to confusion for the user. On #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage, we at least made sure that in Views, when we list languages, we do it in a uniform way and use the names from the Detection Selection settings page for those "derived" languages. But it's still not consistent with the Content Language settings, and the order can be confusing.

We also confuse users by showing them more choices than appropriate.

Anyway, here's the current state [before any patches] of language lists and language names:

a) On the Detection and Selection settings page, the two possibly configurable "language detection" choices are "User Interface text" [configurable by default], and "Content" [not configurable by default].

Screen shot of Detection Selection page

So we should not show the "Content" page language choice in any language lists.

Also note that the terminology on this page is "detection" and "selection". Not "current language" or anything like that.

Also, the name "User interface text" is unnecessarily wordy -- makes some of the choices in lists below very long -- so it could be changed to "Interface text".

b) The language settings from the Content Translation settings page, when you set up a particular content type to be translatable:

Screen shot of Content Language settings

Problems with this list:
- It says "Current interface language", which is hard to relate to what you see on the Detection Selection page, which is "User interface text" and "selected" or "detected", not "current".

Note: This choices list is also visible if you go to edit a content type, in the vertical tabs at the bottom, in the Language tab.

c) If you create a view of a translatable entity (like Node), there is a setting for the Rendering Language of the view:

Screen shot of views render language settings

Problems with this:
- Should not contain "content language selected for page", because this is not currently configurable.
- Order is not consistent with the Content Type language settings.

On the plus side, it does say "selected" and "User interface text" language to match the Detection and Selection page. This is by design: the list is generated from the labels that are defined by the Detection and Selection process, so if these labels change, the Views list will also change.

d) If you create a Views language filter, you get these choices for the value:

Screen shot of choices for vies language filter

This is the same as (c) and generated from the same code, except missing the "row" choices because they do not apply to filters, and with the additional "not specified" language choices, which do not apply to the row rendering language settings.... So we don't need to think about this aside from thinking about (c).

Notes on how to make screen shots

With or without the patch, start up a simplytest.me and do the following to make the screen shots:

  • Standard install profile in English [for faster site building, disable the update checking checkboxes from the config screen so that translations will not be imported -- you don't need them for the screenshots]
  • Enable the 4 multilingual modules - from admin/modules [technically, Config Translation is not required]
  • Add Spanish and French languages - from admin/config/regional/language
  • Go to the Detection and Selection tab and take a trimmed screen shot of this page for (a) - admin/config/regional/language/detection
  • Make Article and Page nodes translatable - from admin/config/regional/content-language - and take a trimmed screen shot of the Default Language choices for (b).
  • Go to the Views page - admin/structure/views - edit the default Content admin view - admin/structure/views/view/content
  • Click on the link next to "Rendering language" and take a trimmed screen shot of the language choices for (c).

Proposed resolution

It would be good if:

1) All language lists were using the same wording for all of the "derived" languages.

2) The "derived" languages that relate to the detected/selected languages for the current page used the same or similar UI text as the detection/selection page.

3) All language lists are as much as possible in the same order.

4) For detection/selection derived languages, hide choices that are not configurable, unless that one has already been selected as the current choice.

Remaining tasks

The current patch makes updates to all of the screen shots above. Here's what it's doing:

a) Detection and selection page: "User interface text" becomes just "Interface text". Screen shot:

Screen shot of language detection/selection page with patch

b) The language choices for content types become:

Screen shot of content type language settings with patch

This is now fixed. The "Current interface language" choice has been renamed "Interface text language selected for page", so it matches (a) well.

c) The language choices for views rendering become:

Screen shot of views render language settings with patch

This is improved:
- The order has been updated so that the derived languages are first
- The order matches the Content settings page (b) as much as possible
- The labels of a couple of the choices have been changed - "User interface text" becomes "Interface text" as in (a)
- Non-configurable choices are hidden [the Content selected language is hidden]

The patch also adds tests to make sure that non-configurable choices are shown if they are the current choice in Views.

User interface changes

The order and labels for various language lists and settings page are more concise and consistent. Non-configurable choices are hidden. The current choice, however, is always shown so that if the list of configurable languages changes, the current choice is still shown.

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because it is a code/UI improvement, not exactly a bug but a consistency improvement.
Issue priority Normal, not critical or major in any way, just a small glitch.
Prioritized changes The main goal of this issue is usability improvement, so it is prioritized.
Disruption Not disruptive. This just updates the UI.
CommentFileSizeAuthor
#93 interdiff_since_58.txt10.7 KBxjm
#86 interdiff.txt1.21 KBPere Orga
#86 2420737-86.patch23.82 KBPere Orga
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,647 pass(es). View
#84 views_lang_withpatch.png17.77 KBjhodgdon
#84 content_lang_withpatch.png16.62 KBjhodgdon
#84 detection_withpatch.png52.39 KBjhodgdon
#82 interdiff.txt4.15 KBPere Orga
#82 2420737-82.patch23.83 KBPere Orga
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,823 pass(es). View
#79 24207370-interdiff-72-79.txt6.55 KBPere Orga
#79 24207370-79.patch19.61 KBPere Orga
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,840 pass(es). View
#75 render_language_withpatch.png16.55 KBjhodgdon
#75 article_settings_withpatch.png16.14 KBjhodgdon
#75 views_filter_nopatch.png15.24 KBjhodgdon
#75 render_language_nopatch.png17.83 KBjhodgdon
#75 content_language_default_settings_nopatch.png19 KBjhodgdon
#75 detection_selection.png30.82 KBjhodgdon
#72 24207370-72.patch15.65 KBPere Orga
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,469 pass(es). View
#72 24207370-interdiff-58-72.txt2.67 KBPere Orga
#69 24207370-interdiff-58-69.diff1.3 KBPere Orga
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 24207370-interdiff-58-69.diff. Unable to apply patch. See the log in the details link for more information. View
#69 24207370-69.patch14.53 KBPere Orga
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,535 pass(es), 1 fail(s), and 0 exception(s). View
#66 language_views_filter_before.png108.42 KBPere Orga
#66 language_views_render_before.png104.6 KBPere Orga
#66 language_content_type_before.png127.78 KBPere Orga
#66 language_content_type_after.png126.55 KBPere Orga
#66 language_views_filter_after.png100.04 KBPere Orga
#66 language_views_render_after.png87.58 KBPere Orga
#58 interdiff.txt851 bytesrodrigoaguilera
#58 24207370-58.patch15.27 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,091 pass(es). View
#56 interdiff.txt845 bytesrodrigoaguilera
#56 24207370-56.patch15.09 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es), 8 fail(s), and 2 exception(s). View
#53 interdiff.txt3.46 KBrodrigoaguilera
#53 24207370-53.patch15.27 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,483 pass(es). View
#48 interdiff.txt7.83 KBrodrigoaguilera
#48 24207370-48-test-only.patch7.98 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es), 10 fail(s), and 1 exception(s). View
#48 24207370-48.patch15.16 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,440 pass(es). View
#46 interdiff.txt5.06 KBrodrigoaguilera
#44 24207370-44.patch12.13 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,502 pass(es). View
#42 interdiff.txt5.06 KBrodrigoaguilera
#42 24207370-40.patch136.49 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,439 pass(es). View
#40 24207370-40-test-only.patch5.06 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,313 pass(es), 11 fail(s), and 7 exception(s). View
#36 interdiff.txt2.13 KBrodrigoaguilera
#36 24207370-36.patch7.07 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,420 pass(es). View
#35 24207370-35.patch6.52 KBrodrigoaguilera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,415 pass(es). View
#35 interdiff.txt1.56 KBrodrigoaguilera
#24 24207370-24.patch6.38 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,133 pass(es), 1 fail(s), and 155 exception(s). View
#24 interdiff.txt5.07 KBGábor Hojtsy
#11 view_language_list.png26.37 KBmaxocub
#11 content_language_list.png26.4 KBmaxocub
#8 interdiff-2420737-5-8.txt2.21 KBmaxocub
#8 languages_list-2420737-8.patch2.98 KBmaxocub
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,292 pass(es). View
#5 2420737-language-names.patch2.74 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,258 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

plach’s picture

Issue summary: View changes
jhodgdon’s picture

Good idea! Are there other places we should be matching too, like the language selection/negotiation page?

The Views list contains an additional option that I don't think is available in the Content settings page: to use the UI text language. Also, when we created the Views list, we were careful to make sure that the two "page" language settings matched the labels in the UI for language selection/negotiation setup; we should not lose this here.

jhodgdon’s picture

But... one other thing: These lists aren't really the same. One is choosing "What language do you want content to be created with, by default", and the other is choosing "Given that a certain piece of content exists in several language, how do you choose which of those translations to display in a view row". I am not sure they should/can be entirely matched up.

Gábor Hojtsy’s picture

Yeah, author's preferred language would not show in the views option list for example. But the dynamic languages that are in both could still be unified in some way.

I think the main contention point here is the negotiated languages. In views we opted to include all that had a title not just those that are configurable, because module developers / site developers may just as well provide something dynamic that is not configurable.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,258 pass(es). View

- Moves dynamic options to top in views.
- Renames interface language option in content config to the same name as in Views.

Still to make further adjustments.

jhodgdon’s picture

I didn't apply the patch, but I think that "Site Default" is lumped in with the static languages in the Views Plugin class. So this patch is moving both Site Default and the static languages to the bottom, which will make the lists still not in the same order.

The fix for the UI text option seems right though.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
maxocub’s picture

FileSize
2.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,292 pass(es). View
2.21 KB

- Moves Site's default before negotiated languages

jhodgdon’s picture

Just noticed this:

+++ b/core/modules/language/src/Element/LanguageConfiguration.php
@@ -97,7 +97,7 @@ public static function processLanguageConfiguration(&$element, FormStateInterfac
   protected static function getDefaultOptions() {
     $language_options = array(
       LanguageInterface::LANGCODE_SITE_DEFAULT => t("Site's default language (!language)", array('!language' => static::languageManager()->getDefaultLanguage()->getName())),
-      'current_interface' => t('Current interface language'),
+      'current_interface' => t('!type language selected for page', array('!type' => 'User interface text')),

'User interface text' in that last line needs to be translated with t() too?

Also in the future, when you upload a new patch:
- Set the status to Needs Review so the test bot and human reviewers know there is something to review/test
- A screen shot of the results would help in reviewing.

maxocub’s picture

Noted for the Needs review and Screen shots (I can't add them right now cause I'm at work).

For the use of t(), I don't know, it was in the previous patch and I think it was copied from the language selection/negotiation setup.

maxocub’s picture

Here are the screen shots.

view language list
content language list

jhodgdon’s picture

Thanks for the screen shot! The new ordering looks good.

So my only concern now is the question about t() in #9. That array('!type' => 'User interface text')), definitely needs to be translated, and currently is not being translated.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Agreed this is the remaining concern with the patch:

+++ b/core/modules/language/src/Element/LanguageConfiguration.php
@@ -97,7 +97,7 @@ public static function processLanguageConfiguration(&$element, FormStateInterfac
+      'current_interface' => t('!type language selected for page', array('!type' => 'User interface text')),

Yeah this should have t() on User interface text, not sure why I did not add it originally.

////

As per the issue summary we may also want to explore whether listing different dynamic language options in the two dropdowns is good. Views has all (even non-configurable types) so long as they have a title. That is good because coders may add types to pick proper languages for some purposes. Also modules can ship with views that use the content language even if the site does not have a separate content language configured. I think that may be a huge plus. On the other hand, it means that it may have confusing options and if things like URL language get a label (see #1994292: LanguageInterface::TYPE_URL (D8) and LANGUAGE_TYPE_URL (D7) have no name or description) then that would be listed too which is even more confusing. (That issue is only blocked on views listing languages if they have labels).

So maybe what we should do is list only configurable types except if the view is using a non-configurable type *already*. Then add that type too, so the configuration is not lost when editing the view?

////

Setting to needs review to send the patch to testbot.

jhodgdon’s picture

Status: Needs review » Needs work

Test bot is happy. Setting back to Needs Work for the t() thing (see #12).

Regarding the question about configurable vs. non-configurable in the Views list... One option would be that we could name them differently and put the non-configurable ones at the end? So it might look like:

Content language of view row
Original language of content in view row
Site's default language (English)
User interface text language selected for page
English
French
Unconfigured selection language: Content language
Unconfigured selection language: URL language

We could also add to the field help a note something like "Language choices include specific languages, languages related to the view row's content, and languages related to the language selection process that is configured on the (link)language selection page(endlink)".

Or something like that... thoughts?

Gábor Hojtsy’s picture

@jhodgdon: its not really unconfigured, its nonconfigurable. You make the content one configurable by enabling that on the language negotiation page, but the URL one can never be made configurable.

jhodgdon’s picture

OK, yeah, I wasn't sure what to label these options, but maybe we can figure something out?

Gábor Hojtsy’s picture

I'd tell you if I'd know a good word for it :) Content language negotiation is non-configurable by default but you can configure it to be configurable (fun). In other words the configurability is configurable :D URL language negotiation cannot be configured to be configurable (in core). And other added methods may or may not be.

jhodgdon’s picture

Is there any way for Views to distinguish between these types of language options?

Gábor Hojtsy’s picture

All we can get from the language info is if they are configurable at the time or not. Whether they have a UI to change that or not is not information we have.

jhodgdon’s picture

Argh. I don't see any way to make this clearer then, or to filter between "choices that make sense to display" and "choices that don't make sense to display".

Gábor Hojtsy’s picture

What about this suggestion I posted in #13?

So maybe what we should do is list only configurable types except if the view is using a non-configurable type *already*. Then add that type too, so the configuration is not lost when editing the view?

This is the same concept we use for shipped config. Shipped config is English so we display a "built-in English" language in the selector even if English is not configured on the site, so you don't loose that association.

jhodgdon’s picture

I think that concept is good, but ... not sure how that would be implemented. We'd have to:
- Add another flag in the views plugin list language function to include just configurable or also non-configurable languages (easy)
- Detect in Views UI whether the current setting is a configurable or non-configurable language, and pass in the right flags (can we do that easily? Probably?)
- Test this.

Gábor Hojtsy’s picture

I don't think thats very hard. We can get a complete list of language types (we already do now) and instead of filtering on title, we filter it on configurability OR whether it is the default value (already set on the view). So you can only set to other configurable types or keep it at the non-configurable one it was.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
5.07 KB
6.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,133 pass(es), 1 fail(s), and 155 exception(s). View

Here is an improvement in that direction. I added two @todos to pass in current values where appropriate. @jhodgdon or someone else maybe you can help ove this along to figure this out. It should not be very hard to do. I postponed #1994292: LanguageInterface::TYPE_URL (D8) and LANGUAGE_TYPE_URL (D7) have no name or description on this one, so it would be nice to resolve this so that can be resolved too. The solution is only complete if the two added @todos are resolved within this issue. Then it also needs tests.

Status: Needs review » Needs work

The last submitted patch, 24: 24207370-24.patch, failed testing.

jhodgdon’s picture

I took a look at the patch and @todos

a) In core/modules/views/src/Plugin/views/display/DisplayPluginBase.php, the list of languages is built in

protected function buildRenderingLanguageOptions()

Looks like that needs to have a parameter $current, which should be OK as it's a protected function. All the callers of this function know what the current option is, so it should not be difficult to pass that in.

Or that method could get it from $this->getOption('rendering_language') directly, since that's the current value in both cases.

b) In core/modules/views/src/Plugin/views/filter/LanguageFilter.php, the list of languages is built in

public function getValueOptions()

We cannot add a current value parameter here... so, let's see. That is deriving from Views base filter classes... Looking at the InOperator class's method valueform(), it looks like you can get the current value from $this->value... and looking at FilterFormatBase, it looks like you can also get it from $this->options['value'], so probably also $this->getOption('value') would work, and that is presumably the right thing to do rather than going directly to the member variables.

Does that help?

Gábor Hojtsy’s picture

andypost’s picture

The related could be useful to split common and special languages

Gábor Hojtsy’s picture

Not sure having this as an autocomplete would be any more useful, probably just more confusing.

penyaskito’s picture

Assigned: Unassigned » penyaskito
Issue tags: +Needs issue summary update

I will work on this one. First thing should be an issue summary update.

penyaskito’s picture

Added "after" screenshots to the IS, but they may be already outdated.
Added proposed resolution based on last comments.
Removed related issue, as an autocomplete would be even more confusing.

penyaskito’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Reordered screenshots.

penyaskito’s picture

Assigned: penyaskito » Unassigned

Unassigned for now.

rodrigoaguilera’s picture

Issue tags: +drupaldevdays

I will give this a try

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
6.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,415 pass(es). View

First let's fix the tests doing the same the @Gábor Hojtsy intented in #24

 $manager->getLanguageTypes();

Doesn't give you any information about the language type so I call $manager->getDefinedLanguageTypesInfo() earlier.

rodrigoaguilera’s picture

Status: Needs review » Needs work
FileSize
7.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,420 pass(es). View
2.13 KB

Took care of the @todos as in #20

For the DisplayPluginBase the option was easily available.

What we wanted in the languageFilter is to get the current values (not options). I found that only available as a public property.

My testing strategy is to check the order of the two selects but I'm not sure about how to test the change in language configuration so $current_values is necessary.

rodrigoaguilera’s picture

Status: Needs work » Needs review
fran seva’s picture

Status: Needs review » Needs work

The code is good for me.
@rodrigoaguilera I'm going to try to figure it out how implements the test. First, as we talked I'm going to try to reproduce manually the case.

Change state to Needs work

rodrigoaguilera’s picture

I'm working on the order tests using xpath to get info from the <select> and <option> elements and checking against expected results.

What I don't know how to test is about the losing of current values. I don't know how to reproduce s and is a bit out of scope of this UX issue.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,313 pass(es), 11 fail(s), and 7 exception(s). View

Added tests that check the order for the rendering language, the language filter and the content translation settings.

Status: Needs review » Needs work

The last submitted patch, 40: 24207370-40-test-only.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
136.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,439 pass(es). View
5.06 KB

And now the complete patch. I added a language filter to the test view so I'm able to test the order of the checkboxes

jhodgdon’s picture

The patch in #42 seems to be for a different issue?? Doesn't seem to be related to the language list, or at least a lot of it isn't?

rodrigoaguilera’s picture

FileSize
12.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,502 pass(es). View

Thanks,
happens when I update my 8.0.x branch and I diff it without rebasing.

The test only patch is the same.

The interdiff is also the same but I upload it again for clarity.

Note again that listing the current values is not completely related to the beginning of the issue but since it deals with the language listing I think it can go in.

rodrigoaguilera’s picture

Status: Needs review » Needs work
rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
jhodgdon’s picture

Wow, this is great! Just a few comments that are very small ... with a working test too!

  1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    @@ -182,6 +182,23 @@ function testSettingsUI() {
         }
    +    // Test that that the order of the languages list is similar to other
    +    // listings like views_ui for UX.
    +    $this->drupalGet('admin/config/regional/content-language');
    

    Nitpick:
    - Probably need a blank line before this section of the test.
    - Comment would be better as:

    Test that the order of the language list is similar to other language lists, such as in Views UI.

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -507,14 +507,17 @@ public function getProvider() {
    +   * @param array|null $current_values
    +   *   The list of current values if available.
        *
    ...
    +  protected function listLanguages($flags = LanguageInterface::STATE_ALL, array $current_values = NULL) {
    

    I usually think this is cleaner as an empty array... but actually... why is it $current_values (plural) instead of one current value? What is the use case where you'd have multiple current values for selecting languages? I think it should probably be just one value?

  3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -523,32 +526,46 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL) {
    +      // Remove site's default from the languages' array so it's not added
    

    nitpick: no ' on languages... how about actually just saying

    Remove site default language from $languages so ...

  4. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -523,32 +526,46 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL) {
    +      foreach ($types as $id) {
    

    I think we need a comment here saying we're only going through the configured types in this loop.

  5. +++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
    @@ -65,7 +65,9 @@ public static function create(ContainerInterface $container, array $configuratio
    +      $this->valueOptions = $this->listLanguages(LanguageInterface::STATE_ALL | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED, array_keys($this->value));
    

    Ah, I see here why the $current_values is plural. Ignore the comment above... although really this is a one-element array, right? It's called 'Language', not 'Languages'...

  6. +++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
    @@ -138,6 +138,42 @@ public function testEditFormLanguageOptions() {
    +        // Check that the order is consistent with other language selectors like
    +        // in admin/config/regional/content-language.
    +        $expected_elements = array(
    

    Modify comment similar to the first item above?

rodrigoaguilera’s picture

Status: Needs review » Needs work
FileSize
15.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,440 pass(es). View
7.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es), 10 fail(s), and 1 exception(s). View
7.83 KB

although really this is a one-element array, right? It's called 'Language', not 'Languages'...

When the current value is a select list is one value, when is a set of checkboxes it might be several languages.

I'm adding a comment on why sometimes is a one element array.

I found out that enabling/disabling the content language negotiation you get a difference on the list of options given for the language types so all the test are possible.

With the new patch I consider everything tested.

Since there is new tests I add another test-only patch.

rodrigoaguilera’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: 24207370-48-test-only.patch, failed testing.

rodrigoaguilera’s picture

Status: Needs work » Needs review

Ready to be reviewed by humans

jhodgdon’s picture

Status: Needs review » Needs work

Very nice work!

I have only a couple of suggestions about the comments and doc to make... and maybe we should have a new After screenshot if it has changed?

I guess I'm a bit confused about the current After screenshot vs. the code, because:
a) In Views I would think now that the Content interface language would not be shown

b) I don't understand the code and comment here, since it looks like I get English still in the languages list, even though it's the default, in the After screenshot:

+    if ($flags & LanguageInterface::STATE_SITE_DEFAULT) {
+      $list[PluginBase::VIEWS_QUERY_LANGUAGE_SITE_DEFAULT] = $this->t($languages[LanguageInterface::LANGCODE_SITE_DEFAULT]->getName());
+      // Remove site default language from $languages so it's not added
+      // with the real languages below.
+      unset($languages[LanguageInterface::LANGCODE_SITE_DEFAULT]);
     }

c) Also I think that English does appear in the list twice for the non-Views UI. Do we want it there?

Other review comments:

  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -507,14 +507,17 @@ public function getProvider() {
    +   * @param array|null $current_values
    +   *   The list of current values if available so we add them to the list.
        *
    

    Hm, these docs are a bit confusing.

    My suggestion is to have the @param docs just say:

    The currently-selected options in the list, if available.

    And then in the return we should make sure we document it's only the configurable languages plus current. How about adding this to the end of the @return docs that are already there:

    Only configurable languages and languages that are in $current_values are included in the list.

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2609,7 +2609,9 @@ public function getExtenders() {
    +    // Pass the current rendering language (in this case a one element array) so
    +    // is not lost when there is language configuration changes.
    +    return $this->listLanguages(LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED | PluginBase::INCLUDE_ENTITY, array($this->getOption('rendering_language')));
    

    Nitpick: is -> are in second line

  3. +++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
    @@ -65,7 +65,9 @@ public static function create(ContainerInterface $container, array $configuratio
    +      // Pass the current values so options that are already selected do not get
    +      // lost when there is changes in the language configuration.
    +      $this->valueOptions = $this->listLanguages(LanguageInterface::STATE_ALL | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED, array_keys($this->value));
    

    Same here

rodrigoaguilera’s picture

FileSize
15.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,483 pass(es). View
3.46 KB

I have only a couple of suggestions about the comments and doc to make... and maybe we should have a new After screenshot if it has changed?

The screenshots haven't changed

I guess I'm a bit confused about the current After screenshot vs. the code, because:
a) In Views I would think now that the Content interface language would not be shown

Like it happens in the test it only makes sense to list it if you enable the content language negotiation, the screenshots are about the order not about what options are available.

b) I don't understand the code and comment here, since it looks like I get English still in the languages list, even though it's the default, in the After screenshot:

Do you refer to the English listed as a default language (this can change) and English listed as a fixed language?
In that case makes sense to list it twice because the default language can change. Otherwise I don't get your confusion. I think few people want to change the default language of a site after is built.

c) Also I think that English does appear in the list twice for the non-Views UI. Do we want it there?

Isn't the same case as b) ?

Upload new patch with comment changes

rodrigoaguilera’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

@rodrigoaguilera and I chatted at DDD and concluded:

a) Point a from #52 - right! The Content language is configurable, so it should appear in Views UI. But it doesn't make sense when defining default language for new content, because there is no way to have a content language before the content exists, duh!

b) from #52 - It's these three lines:

+      // Remove site default language from $languages so it's not added
+      // with the real languages below.
+      unset($languages[LanguageInterface::LANGCODE_SITE_DEFAULT]);

This seems to be saying that if the default language is English, English will appear as "Site default language (English)" and then be removed from the list of fixed languages at the bottom.

However:
- I am not sure that is what we want to happen.
- If it *is*, it's not working.

So we should either fix this so it actually works, or remove these three lines. My thought is to just remove the lines, because at least in Views, saying "Make this display in default language" is not really fixed to be English, it's just *currently* English according to your settings. But saying "Make this display in English" is saying no matter what the site settings are, use English. So they really are two different values in a views config.

And yes, (c) is the same as (b), except applied to the non-Views-UI, so I think the answer is the same there: don't do anything to the code there and have it twice.

====> Remove those 3 lines.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
15.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es), 8 fail(s), and 2 exception(s). View
845 bytes

Removed the 3 lines

Status: Needs review » Needs work

The last submitted patch, 56: 24207370-56.patch, failed testing.

rodrigoaguilera’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,091 pass(es). View
851 bytes

Nope.
That line that I removed in the last patch prevented the default language to be added twice. I cleared a bit the comment for that line.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ah, I see. Better comment. This is RTBC then. Hooray for consistency!

Adding beta eval to issue summary.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests +Needs usability review, +Needs issue summary update

Hmm, I'm not sure about this change. I actually find the "after" to be more difficult to understand in some ways. The list is difficult to scan, and the ordering seems almost random. I've tried understanding why the order is what it is by reading the issue, but it's still not clear. A few specific questions:

  1. Is the fact that Italian is present in the "Before" screenshots but not the "After" meaningful, or a difference in the test site configurations?
  2. How is "Content language of view row" different from "Content language selected for page"? Which page? A page display? A node (which is not necessarily a page node)?
  3. Why doesn't "Content language selected for page" appear in the options for the content default language? (The answer to #2 might explain this.)
  4. The list is very hard to scan and understand. Would optgroups be an option to make it more understandable?
  5. I had the impression that this patch would affect which "non-configurable" languages appeared in the list but it doesn't seem to from the screenshots. Can someone put more detail explaining that part of it in the summary?
  6. +++ b/core/modules/language/src/Element/LanguageConfiguration.php
    @@ -97,7 +97,7 @@ public static function processLanguageConfiguration(&$element, FormStateInterfac
    -      'current_interface' => t('Current interface language'),
    +      'current_interface' => t('!type language selected for page', array('!type' => t('User interface text'))),
    

    Is this actually an improvement? This seems to be changing the meaning of the option, and having more words to parse and understand could be considered a usability regression, one that seems like we're adding to the Language module to make it consistent with Views rather than the other way around.

  7. This isn't changed by the patch, but I guess the difference between "Content language of view row" and "Original language of content in view row" is that the former is the translation, if any, while the latter is specifically the original language and not the translation. What's the usecase for that extra option?
  8. I understand why "Author's preferred language" is not an option for the view row since the content has already been authored, but that option disappearing from the middle of the list (rather than the beginning or end) makes the re-ordering less valuable as a UX improvement.

Overall, I'd like a usability review for this; let's see if we can come up with something better. Also, let's update the summary with a more detailed explanation of the current patch. Explaining what each of the options that appear in both the content default and views lists mean will also help usability reviewers make a recommendation, if appropriate.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

xjm: The point of this issue was to make the language lists in Views be the same as the language lists in the content translation UI. The way things are now, the two are not using the same names for the same "special" languages, and they're not in the same order, so users will be confused.

This patch improves things so that the two language listings are at least consistent with each other. These are both in UIs for advanced site builders: in settings for content type translation and in setting up Views. I don't really think we need to debate and bike shed for ages on what order to make them, and we already had the debate as to what to call them on other issues when we set up the Views languages stuff.... I am going to go ahead and put this back to RTBC...

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Again, I don't agree that the patch actually improves the situation. I think it might actually make it worse, for Views, for Language, and for usability generally. See specifics in #60.

penyaskito’s picture

Pinged our UX team for trying to get help moving forward (https://twitter.com/penyaskito/status/593533376025374720)

danigrrl’s picture

I would need more context on use case, when someone would see this message, etc. to make a specific recommendation, but I'd agree with @xjm that the above options, as it pertains to Views, aren't really clear. A couple of quick notes are here: https://www.evernote.com/shard/s4/sh/12c6a864-eb87-41f1-a02a-28074a4011d....

But the basics are:

  1. What is the most likely option the user would choose? I would imagine that it would be either "Site's Default Language" or "User Interface Text Language"
  2. (SIDE NOTE: you can get rid of the word "user" in both areas; people will get what Interface Text means, and it's easier to fit in the area).
  3. I honestly don't understand what the first two options pertain to. The explanations I've seen in comments don't help; they look like they mean the same thing, and they're likely to confuse people more than help.
  4. I believe what you might be trying to say is "Current Language of content in row" and "Original Language of content in row;" is that accurate?
  5. What is the definition of "page" in this list? Is it page as in "This display, which is named Page," or is it something else? This has implications for understandability.

In terms of simply moving this ticket forward, I'd recommend moving Site's Default Language and User Interface Text Language to the top of this list, and making "Site's Default Language" the default. The exception to this would be if the descriptions in item #4 are accurate; in that case, I'd recommend changing the labels to what has been recommended here, and making "Current Language of Content in Row" the default.

Hope that helps!

Bojhan’s picture

Is the goal here to improve the UX, or simply to make it more consistent. The latter we achieve, for the former - I would suggest a new issue as xjm is correct here.

Pere Orga’s picture

1. Is the fact that Italian is present in the "Before" screenshots but not the "After" meaningful, or a difference in the test site configurations?

It doesn't look meaningful to me, updating the screenshots (including changes suggested below). Also adding screenshot of list of languages for views filtering. Trying to improve the issue summary too.

2. How is "Content language of view row" different from "Content language selected for page"? Which page? A page display? A node (which is not necessarily a page node)?

"Content language of view row" and "Content language selected for page" are unrelated. The first is default option to refer to the language of the content in the views results (from the outputted entities) and the second is the negotiated language of the page (e.g. "English" when <html lang='en'>). Page here means the rendered page output.
So "Content language of view row" is specific to the views row, it ensures the returned entities will be displayed on its language (by default).
"Content language selected for page" will make Views try to show the content in the language of the page, if a translation exist.

3. Why doesn't "Content language selected for page" appear in the options for the content default language? (The answer to #2 might explain this.)

It may be because is not possible to have the content language as a default language for new content (when creating entities). It does not make sense to me.

4. The list is very hard to scan and understand. Would optgroups be an option to make it more understandable?

Not sure, it could be even worse.

5. I had the impression that this patch would affect which "non-configurable" languages appeared in the list but it doesn't seem to from the screenshots. Can someone put more detail explaining that part of it in the summary?

The issue summary says "List only configurable types except if the view is using a non-configurable type *already*. Then add that type too, so the configuration is not lost when editing the view." but also, I can't see this changed looking at the code.

6. Is this actually an improvement? This seems to be changing the meaning of the option, and having more words to parse and understand could be considered a usability regression, one that seems like we're adding to the Language module to make it consistent with Views rather than the other way around.

Agreed. "language selected for page" does not help in the Field UI. Maybe, for UX consistency, could be changed on Views instead.

7. This isn't changed by the patch, but I guess the difference between "Content language of view row" and "Original language of content in view row" is that the former is the translation, if any, while the latter is specifically the original language and not the translation. What's the usecase for that extra option?

That's right. In the case content has been translated at least once, you may want to display the original content. Not super useful but IMHO worth keeping it. But it should only appear on Views.

8. I understand why "Author's preferred language" is not an option for the view row since the content has already been authored, but that option disappearing from the middle of the list (rather than the beginning or end) makes the re-ordering less valuable as a UX improvement.

I don't see the use case to add this on Views. If nobody is going to use that, adding it could be considered a usability regression.

Pere Orga’s picture

Issue summary: View changes
Pere Orga’s picture

1. What is the most likely option the user would choose? I would imagine that it would be either "Site's Default Language" or "User Interface Text Language"

On Views UI, I think it would be "Content language of view row". On Field UI pages, I think it would be "User Interface Text Language".

2. (SIDE NOTE: you can get rid of the word "user" in both areas; people will get what Interface Text means, and it's easier to fit in the area).

I'm not sure if we should remove "user", but shouldn't we remove "text"? I think "Interface Language" is better than "Interface Text Language".

3. I honestly don't understand what the first two options pertain to. The explanations I've seen in comments don't help; they look like they mean the same thing, and they're likely to confuse people more than help.

4. I believe what you might be trying to say is "Current Language of content in row" and "Original Language of content in row;" is that accurate?

Yes, it is. I think keeping the word "view" gives some context. Also, I would remove the word "Current" because that would make it more confusing (Current language is usually used for the current language of the page). I suggest changing "Content language of view row" to "Language of content in view row" to make it more consistent with "Original language of content in view row" - is also not used anywhere else. I'm not sure that would be better, and in any case it can be further improved.

5. What is the definition of "page" in this list? Is it page as in "This display, which is named Page," or is it something else? This has implications for understandability.

Here, "page" is the HTML the browser receives.

Pere Orga’s picture

FileSize
14.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,535 pass(es), 1 fail(s), and 0 exception(s). View
1.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 24207370-interdiff-58-69.diff. Unable to apply patch. See the log in the details link for more information. View

Reattaching the patch in #58 with the following differences:
1. Changes "Content language of view row'" to "Language of content in view row" in the Views UI
2. Does not change the text of "Current interface language" in the Field UI

Not sure if we should change the state "needs work" for this (probably), but keeping it to "needs review" for now

Status: Needs review » Needs work

The last submitted patch, 69: 24207370-interdiff-58-69.diff, failed testing.

The last submitted patch, 69: 24207370-69.patch, failed testing.

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
15.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,469 pass(es). View

Forgot to update test

Gábor Hojtsy’s picture

@xjm / @danigrrl: the difference in ordering of the options is exactly to move the most common ones to the top for each scenario. Answering each point in @xjm's and @danigrrl's review although @Pere Orga responded, these answers may help clarify and also deepen the understanding of the options, so we can move forward hopefully.

Is the fact that Italian is present in the "Before" screenshots but not the "After" meaningful, or a difference in the test site configurations?

Should be just a site difference, not a patch difference.

How is "Content language of view row" different from "Content language selected for page"? Which page? A page display? A node (which is not necessarily a page node)?

The first one is the language of the content in the row, the later one is the language selected for content display in general in the request. I don't think we want to use technical words like "request" on the UI? So that is why we use page.

Why doesn't "Content language selected for page" appear in the options for the content default language? (The answer to #2 might explain this.)

It could. We choose to display only configurable language options for content default language and choose to display options with labels in views. These result in a different subset. Reason being so modules can ship with views that depend on language types that may not be configurable on your site. Or that you can add a language type that is not configurable and use it in views. This later use case may also apply to content defaults but not really to default content language.

The list is very hard to scan and understand. Would optgroups be an option to make it more understandable?

I think the different groups of things is pretty technical. The groups are things like "Concrete configured languages on your site", "Locked languages shipped with Drupal / modules", "Languages computed based on this view row", "Languages computed on the request level". Not sure how to come up with meaningful labels for those groups for users to understand better?

I had the impression that this patch would affect which "non-configurable" languages appeared in the list but it doesn't seem to from the screenshots. Can someone put more detail explaining that part of it in the summary?

The after screenshot does not have the content language selected for page option, which is the non-configurable language that was appearing here but not for content defaults, so look like this was resolved. You maybe understood what a non-configurable language was differently?

+++ b/core/modules/language/src/Element/LanguageConfiguration.php
@@ -97,7 +97,7 @@ public static function processLanguageConfiguration(&$element, FormStateInterfac
-      'current_interface' => t('Current interface language'),
+      'current_interface' => t('!type language selected for page', array('!type' => t('User interface text'))),

Is this actually an improvement? This seems to be changing the meaning of the option, and having more words to parse and understand could be considered a usability regression, one that seems like we're adding to the Language module to make it consistent with Views rather than the other way around.

Language types are extensible, so there may be multiple of them. I think this was meant as a unification of terminology/option labeling which accommodates the extensibility.

This isn't changed by the patch, but I guess the difference between "Content language of view row" and "Original language of content in view row" is that the former is the translation, if any, while the latter is specifically the original language and not the translation. What's the usecase for that extra option?

Which one is extra? The most common option likely is the content language of the view row, we made core views to default to that in fact and this is the reason we list that as the first option. As for why there is an option to render it in the original language, this is varied based on the content, so you cannot achieve the same behavior with any of the other options. If you are to create a translator dashboard (workbench style) for example, then you would include possibilities like "List me content that is not yet translated to French" where you would need to display it in the original language (which may not be the same across all content found, so none of the other options let you make sure you rendered in a language that is available to show).

I understand why "Author's preferred language" is not an option for the view row since the content has already been authored, but that option disappearing from the middle of the list (rather than the beginning or end) makes the re-ordering less valuable as a UX improvement.

Well, as per @danigrrl we would order the items in order of common use. I would argue that defaulting to Not applicable is less common than defaulting content to the author's preferred language. So this option would be somewhere in the middle of the list due to that ordering criteria alone.

What is the most likely option the user would choose? I would imagine that it would be either "Site's Default Language" or "User Interface Text Language"

Contrary to what Pere Orga suggested, I think the content of the view row is the ultimate default for views (which is why its first) and site's default is indeed the logical default for content default language (which is why its first). If we would make site's default the top-most option for views and select out of the box, then views created would not work on multilingual sites at all. We default to content language of views row so views work equally well on single language and multilingual sites out of the box.

(SIDE NOTE: you can get rid of the word "user" in both areas; people will get what Interface Text means, and it's easier to fit in the area).

This makes a lot of sense indeed. Looks like we don't use the word "user" much in relation to interface text language, just in the method, so would be relatively easy to fix:

$ git grep ".ser .nterface .ext"
core/lib/Drupal/Core/Language/LanguageManager.php:        'name' => $this->t('User interface text'),
core/lib/Drupal/Core/Language/LanguageManager.php:        'description' => $this->t('Order of language detection methods for user interface text. If a translation of user interface text is available in the detected language, it will be displayed.'),
core/modules/language/language.admin.js:        // Get the language detection type such as User interface text language
core/modules/language/language.module:      'name' => t('User interface text'),
core/modules/language/language.module:      'description' => t('Order of language detection methods for user interface text. If a translation of user interface text is available in the detected language, it will be displayed.'),
core/modules/language/src/Form/NegotiationConfigureForm.php:        '#title' => $this->t('Customize %language_name language detection to differ from User interface text language detection settings', array('%language_name' => $info['name'])),
core/modules/locale/src/Form/TranslateEditForm.php:            // Simplify user interface text for the most common case.
core/modules/views/src/Plugin/views/field/NumericField.php:      // Simplify user interface text for the most common case.

I honestly don't understand what the first two options pertain to. The explanations I've seen in comments don't help; they look like they mean the same thing, and they're likely to confuse people more than help.

I assume this was in reference to the views options. You can either render the content in the language found or in the language the content was originally created (before possibly being translated to the language it was found in). See above for why do we have the two options. If you are to have a view to list content based on uncertain translation availability (especially to list content that is NOT available in some language), then the only language you can be sure of is available for each piece of content found is the original language it was created in. You don't know ahead of time if there was any other translations that you could render in. This makes the results consistent in the view.

I believe what you might be trying to say is "Current Language of content in row" and "Original Language of content in row;" is that accurate?

I think so yeah. I don't see the difference in meaning from the prior text, but that is *definitely* due to my ignorance due to my background knowledge, sorry for that.

What is the definition of "page" in this list? Is it page as in "This display, which is named Page," or is it something else? This has implications for understandability.

The options in this list use page where they would technically need to say HTTP request.

Pere Orga’s picture

Status: Needs review » Needs work

Contrary to what Pere Orga suggested, I think the content of the view row is the ultimate default for views (which is why its first) and site's default is indeed the logical default for content default language (which is why its first). If we would make site's default the top-most option for views and select out of the box, then views created would not work on multilingual sites at all. We default to content language of views row so views work equally well on single language and multilingual sites out of the box.

No, that's what I was trying to say :)

I'm understanding that my changes in #69 should to be reverted.

jhodgdon’s picture

Issue summary: View changes
FileSize
30.82 KB
19 KB
17.83 KB
15.24 KB
16.14 KB
16.55 KB

I have completely redone the issue summary and screen shots for this issue, to clarify what it does and what it doesn't do.

For this purpose, I created two test sites on simplytest.me, one without the patch and one with the patch:
- Standard install
- Enable the 4 language modules.
- Add Spanish and French
- Make Article and Page nodes translatable
and I made some new screen shots.

So... According to what I just wrote in the issue summary, I think this is Needs Work. Namely, I think that the Content language setting that means "use the current UI language" should be labeled in a way that is consistent with Views and consistent with the Detection Selection page. Currently it says "Current interface language", whereas in Views we are saying "User interface language selected for page". I think the Views one is more consistent with the Detection Selection page.

Gábor Hojtsy’s picture

@jhodgdon: so the first hunk of #69 should be rolled back and/or a shorter common identifier label should be figured out. See also #60/6 and #64/2.

jhodgdon’s picture

@Pere Orga: Do you have time to work on this, or would you like me to make the next patch?

@Gabor I am not sure what you are saying should be rolled back. The first hunk of the patch in #69 is a test, as far as I can see.

I also strongly disagree with #60 item 6. Something that says "Current interface language" does not match what you see on the Detection Selection page with what is in the language list. The Detection page calls it the "User interface text" language, and "current" does not match "selected". So I think that changing "Current interface language" to "User interface language text selected for page" is a better match, even though it is more wordy.

Or if we want to change it to be "Interface text", which would be fine with me, as suggested in #64/2, then I think we should change this on the Selection Detection page too, to make it all consistent.

But we should get rid of the word "Current", in favor of always calling it "Selected", I think.

So....

I think what needs to be done, from the current patch, is:

a) On the Detection Selection page, change the name of "User interface text" language to just "Interface text".
===> This will automatically change the Views language lists to use this name.

b) Make sure that the langauge choices that mean "the language that was negotiated/selected/detected for UI text" all say:
==> Interface text language selected for page
[not "Current interface language"]
===> This needs to be changed on the content type language selection code, and it should always be derived from the actual label the way we do it in Views, so that if we change the label on Detection Selection again it will automatically be updated.

==> Unless we can think of a more compact wording, but it needs to contain the phrase "Interface text" and the word "selected". Not "current", because that doesn't match the Detection Selection page.

Right?

Pere Orga’s picture

@Pere Orga: Do you have time to work on this, or would you like me to make the next patch?

I may work on this, feel free to take it.

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
19.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,840 pass(es). View
6.55 KB

Reverting #69.1: From "Language of content in view row" to "Content language of view row" again
#64.2: Changed "user interface text" to "interface text", everywhere.

Can we agree on the next steps?

Pere Orga’s picture

If you want me to revert #69.2 instead of #69.1, or both, please advise

Gábor Hojtsy’s picture

@jhodgdon: I meant the first hunk of the *interdiff* in #69 sorry. That seems to be happening now.

@Pere Orga: I think the changes make sense, but do not cover yet all the suggestions from @jhodgdon in #77. I agree with @jhodgdon in #77 btw.

Pere Orga’s picture

FileSize
23.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,823 pass(es). View
4.15 KB

Ok done.

Gábor Hojtsy’s picture

Title: Improve UX of views language list » Differences in dynamic language names are confusing in views, content, etc.

Retitled based on the current scope given that fixing the views language lists required changes for consistency elsewhere too. Needs issue summary update again due to the changes in strings since #75.

jhodgdon’s picture

Issue summary: View changes
FileSize
52.39 KB
16.62 KB
17.77 KB

OK, I have updated the summary with new screen shots/text. Also put notes in summary as to how to make a test site for screen shots.

I think this patch is good, functionality wise! I haven't looked at the code, but I am +1 for RTBC based on testing it.

jhodgdon’s picture

Status: Needs review » Needs work

Nitpicky review of the code:

  1. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationUI.php
    @@ -11,7 +11,8 @@
    - * Class for identifying the language from the current interface language.
    + * Class for identifying the language from the interface text language selected
    + * for page.
      *
    

    Classes always need to start with a one-line description. So this needs to be shortened.

    I suggest removing "class for", so this would be:

    Identifies the language from ...

  2. +++ b/core/modules/views_ui/src/Tests/UITestBase.php
    @@ -35,6 +35,9 @@
    +  /**
    +   * {@inheritdoc}
    +   */
       protected function setUp() {
         parent::setUp();
    

    I don't think we're actually putting doc blocks on test setUp() methods are we? Maybe check to see if other tests in Core have them, and remove this if not.

The code looks really good... probably need to fix these very very minor issues and then as far as I can tell we are good to go! Thanks!!

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
23.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,647 pass(es). View
1.21 KB
  1. I suggest removing "class for", so this would be:

    done

  2. I don't think we're actually putting doc blocks on test setUp() methods are we? Maybe check to see if other tests in Core have them, and remove this if not.

    We have a few cases:

    $ git grep -B2 'function setUp()' * | grep inheritdoc | wc -l
         527
  3. Additionally, fixed typo in comment

Status: Needs review » Needs work

The last submitted patch, 86: 2420737-86.patch, failed testing.

Pere Orga queued 86: 2420737-86.patch for re-testing.

Pere Orga’s picture

Status: Needs work » Needs review

Test bot was momentarily drunk

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -blocker, -Needs usability review, -Needs issue summary update

Great, thanks! This is I think now ready to go. Issue summary is updated and beta evaluation is added.

If we need to make further changes for usability, we should file another issue, but I'm removing the usability review tag as that has been done and the concerns addressed. The goal here is to have everything consistent, which is accomplished, and to some extent the choices are made more compact. If we need them to be even better, we'll need to bikeshed/discuss what they can be to still be consistent.

Also removing the blocker tag, because I don't see any issues this is blocking.

Gábor Hojtsy’s picture

Issue tags: +blocker

@jhodgdon: see #27 where the blocker tag was added.

jhodgdon’s picture

Ah, well we should add an issue relationship then.

xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup
FileSize
10.7 KB

Thanks everyone for the detailed clarifications and thanks @jhodgdon for the clear issue summary and screenshots. It's much easier to wrap my head around the intent of the patch now (though I still have trouble wrapping my head around the options themselves!). ;)

I attached an interdiff for the changes since #58, which in terms of the two select boxes are just changing "User interface text" to "Interface text" (and then standardizing on that elsewhere in the UI and codebase).

There was a dorky merge conflict because of #2491155: Update drupal.org and kernel.org URLs in core modules (Follow-up to 2489912):

++<<<<<<< ours
 +    // @todo Consider making these plugins. See
 +    //   https://www.drupal.org/node/2173811.
 +    return $this->listLanguages(LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED | PluginBase::INCLUDE_ENTITY);
++=======
+     // @todo Consider making these plugins. See https://drupal.org/node/2173811.
+     // Pass the current rendering language (in this case a one element array) so
+     // is not lost when there are language configuration changes.
+     return $this->listLanguages(LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED | PluginBase::INCLUDE_ENTITY, array($this->getOption('rendering_language')));
++>>>>>>> theirs

Resolved with:

diff --cc core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
index bc85ef7,1cdf0ef..0000000
--- a/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@@ -2607,9 -2607,10 +2607,11 @@@ abstract class DisplayPluginBase extend
     *   An array of available entity row renderers keyed by renderer identifiers.
     */
    protected function buildRenderingLanguageOptions() {
 -    // @todo Consider making these plugins. See https://drupal.org/node/2173811.
 +    // @todo Consider making these plugins. See
 +    //   https://www.drupal.org/node/2173811.
-     return $this->listLanguages(LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED | PluginBase::INCLUDE_ENTITY);
+     // Pass the current rendering language (in this case a one element array) so
+     // is not lost when there are language configuration changes.
+     return $this->listLanguages(LanguageInterface::STATE_CONFIGURABLE | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED | PluginBase::INCLUDE_ENTITY, array($this->getOption('rendering_language')));
    }

Here are a few outstanding questions that (as @jhodgdon points out) can probably be handled in followups:

  1. In general, I still find all this confusing and overwhelming. I looked through the hook_help() for content_translation.module, language.module, and locale.module, but still find it quite confusing. Could we add a list somewhere (like in a hook_help()) of what each of these options actually are, and link to it in the select field's description or such for each of (b), (c), and (d) from the summary? I think this is worth a followup issue.
     

  2. Regarding the option for:
    Interface text language selected for page

    @Gábor Hojtsy explained in #73:

    The first one is the language of the content in the row, the later one is the language selected for content display in general in the request. I don't think we want to use technical words like "request" on the UI? So that is why we use page.

    Okay, that makes sense. I'm still concerned about this being confused with page displays and page nodes, though, as @danigrrl also highlighted.

    I also am confused as to how the language was "selected" (by whom? where? how?). If it were the language configured for the entity or a translation thereof, or something the site user clicked on, that would make sense, but how is it "selected" for a particular request? "Selected" seems to imply human interaction, but "for the request" implies some sort of automatic calculation.

    It looks like the word "selected", "selection" is used elsewhere (looking at the "Detection and Selection settings" title and language_help(), so I get now that this is a specific term and changing it would be out of scope here.

    If folks are satisfied with this option as it is in the patch (and since the patch does accomplish the goals of making it consistent), could we do a followup issue to explore potential alternatives for "page" and for "selected"?
     

  3. I also still don't quite get the difference between
    Content language of view row
    and
    Original language of content in view row

    @Gábor Hojtsy said in #73:

    As for why there is an option to render it in the original language, this is varied based on the content, so you cannot achieve the same behavior with any of the other options. If you are to create a translator dashboard (workbench style) for example, then you would include possibilities like "List me content that is not yet translated to French" where you would need to display it in the original language (which may not be the same across all content found, so none of the other options let you make sure you rendered in a language that is available to show).

    and:

    You can either render the content in the language found or in the language the content was originally created (before possibly being translated to the language it was found in). See above for why do we have the two options. If you are to have a view to list content based on uncertain translation availability (especially to list content that is NOT available in some language), then the only language you can be sure of is available for each piece of content found is the original language it was created in. You don't know ahead of time if there was any other translations that you could render in. This makes the results consistent in the view.

    So I don't know what the "language found" means still even after reading over this for an hour. :( Is the idea that the view could be a view of (e.g.) German translations, which would be configured somewhere in another Views handler, and the first option would display the German translation in that case? If there were a word in the first option that contrasted with the second, rather than there just being an added word, maybe that would help?

    This would be worth its own separate followup, since it's Views-specific.
     

  4. +++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
    @@ -138,6 +138,73 @@ public function testEditFormLanguageOptions() {
    +        // Test that the order of the language list is similar to other language
    +        // lists, such as in Views UI.
    

    Minor: This is the test for the Views UI. I think this comment should be "in the content translation settings" or such? I was going to just fix this on commit but my brain has melted and I also am not sure of the proper label for the... thingy. So this is a tiny (and novice-friendly) followup to make.
     

So, four followups please. :)

This issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. Committed and pushed to 8.0.x.

  • xjm committed 50cfc9e on 8.0.x
    Issue #2420737 by rodrigoaguilera, Pere Orga, Gábor Hojtsy, maxocub,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Opened #2494907: Explain and improve the usability of dynamicly selected languages for the first three. I think if we are to write help text for options in one and rename them in another, we can just as well do them in one (and evaluate if the help text is still needed if we are to rename to more sensible names). Posted even more background there.

Opened #2494915: Fix comment in ViewEditTest.php for 3 and provided a patch. Review welcome. Your suggestion was the correct one.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

maxocub’s picture