Problem/Motivation
This is a follow-up to #1831530: Entity translation UI in core (part 2)
User interface changes
Only show source translation column if there are 2 or more source languages (not including "n/a").
Original report from phone call
see #1188388-181: Entity translation UI in core
Problem/Motivation
This is a follow-up to #1831530: Entity translation UI in core (part 2)
Proposed resolution
Only show source translation column if there are 2 or more source languages (not including "n/a").
On the "translate" tab of a node, users currently see a "Source translation" column regardless of whether they have more than one source language for the node. This column should not be shown unless there are more sources than just the original language and the "n/a" label.
Example of one source language shown before:
Example of two source languages shown before:
Example of one source language shown after:
Example of two source languages shown after:
Remaining tasks
- done - add screenshot of current #5
- done - agree on new UI
- done - create patch after ui is agreed on
- (novice) test patch, see steps to test in #23. Contributor Task for manual testing: http://drupal.org/node/1489010
- (novice) review patch. Contributor Task for review: http://drupal.org/node/1488992
- (novice) write automated tests. Contributor Task for tests: http://drupal.org/node/1468170
User interface changes
Only show source translation column if there are 2 or more source languages (not including "n/a").
API changes
no expected API changes
Original report from phone call
Comment | File | Size | Author |
---|---|---|---|
#107 | 1832870-107.patch | 5.23 KB | robertdbailey |
#107 | 1832870-107-TESTS-ONLY-SHOULD-FAIL.patch | 2.86 KB | robertdbailey |
#107 | interdiff-1832870-100-107.txt | 1.68 KB | robertdbailey |
#2 | ss1-1832870.png | 161.28 KB | maryedith |
#5 | drupal-only_display-source_languae_for_multiple_sources-1832870-5.patch | 1.73 KB | -enzo- |
Comments
Comment #1
maryedith CreditAttribution: maryedith commentedComment #1.0
maryedith CreditAttribution: maryedith commentedupdated with comment #181 info
Comment #1.1
maryedith CreditAttribution: maryedith commentedupdated summary with task list
Comment #1.2
maryedith CreditAttribution: maryedith commentedUpdated issue summary. Fixed typo
Comment #1.3
maryedith CreditAttribution: maryedith commentedUpdated issue summary. Fixed typo
Comment #2
maryedith CreditAttribution: maryedith commentedComment #3
YesCT CreditAttribution: YesCT commented@maryedith Thanks for posting this. A good issue summary is really helpful.
Next steps:
I think it would help to have two screen shots to clarify this.
One screenshot would illustrate when the column would be not shown, along with 1 or two sentences explaining why it's better to hide it.
Another screenshot would be an example of when it would be shown.
Comment #4
-enzo- CreditAttribution: -enzo- commentedWorking in this issue
Comment #5
-enzo- CreditAttribution: -enzo- commentedHello folks
I did a patch to hide the column if the entity only have some source language
Entity with one unique language source: English
Entity with two language source : English & Spanish
Comment #5.0
-enzo- CreditAttribution: -enzo- commentedUpdated issue summary. Fixed typo
Comment #7
-enzo- CreditAttribution: -enzo- commented#5: drupal-only_display-source_languae_for_multiple_sources-1832870-5.patch queued for re-testing.
Comment #8
Bojhan CreditAttribution: Bojhan commentedsmart usability++
Comment #9
plachOverall the patch looks good, thanks!
However, there are many coding standard issues that need to be fixed before this can go in:
Missing space after "//" and trailing dot.
Missing space after "if".
The "else" keyword should be on its own line.
Trailing whitespaces.
Missing space after "foreach". Also "$idx" is not a very common variable name for a numeric index: I'd use, well, "$index" or something similar :)
Comment #10
plachComment #11
-enzo- CreditAttribution: -enzo- commentedFixed patch to apply code standards
Comment #12
plachThere's still a trailing whitespace left.
No chance to get a more readable name here? :(
Comment #13
-enzo- CreditAttribution: -enzo- commentedHello plach
I didn't the fix patch for that line following the idea of webchick to maintain the context http://webchick.net/please-stop-eating-baby-kittens.
For the name , you can give the your idea for a better name?
Comment #14
plachWell, that article speaks about focusing on fixing a single issue in a patch, but it refers to existing code. The trailing whitespace is being introduced by the patch so it needs to be fixed.
$index :)
Comment #15
-enzo- CreditAttribution: -enzo- commentedChanged applied
Comment #16
YesCT CreditAttribution: YesCT commentedMaybe it's more standard to use ++.
The variable $source_languages does not really contain the source languages, it's a count of them, which is made clear later.
I think adding a comment for this block would help like:
// Count the number of source languages.
or is it really:
// Record the number of times each language is used as a source for a translation.
and/or
change the variable name to number_of_ ...
would help too.
I looked for an example elsewhere in core, and in
core/modules/file/file.module
there is something like
$existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));
if (count($existing_files)) {
enzo is in irc. I'll ask there.
comment says less than 2, but the code says 3.
Also, it's removing just one column so it should be:
Remove Source Language column if there are less than two source languages.
Comment #17
YesCT CreditAttribution: YesCT commentedbackground to explain counting the number of a source is used in translations
it's equivalent to the number of rows in which a source language would appear.
Its a little strange because in, for example, a table like the one below,
$count_lauguage_as_source['n/a']
would be 3 (there are 3 rows with n/a as the source)$count_lauguage_as_source['English']
would be 1$count_lauguage_as_source['Afrikaans']
would be 1and
count($count_lauguage_as_source)
is 3 (there are 3 values of source language in the table: n/a and two other languagespickiness about variable names
I changed the variable name to $count_language_as_source[] to attempt to accurately describe that it is number of translations/rows in the table for each value of the name of a source language. But I think a better var name could be picked if we redo this again.
We dont really need to know how many rows have English as a source.. we just need to know that English is used as a source. So we could do true false that it's used instead of counting them. We dont really care that there are 5 rows all with n/a. We just care that n/a is the only element in the array, and so there were less than two unique source languages and that means we dont need the source column. [edit: added this paragraph]
It's weird to have '2' in the if for the number of sources deciding when to remove the column, and then later again to unset($header[2]); because those are not the same in meaning, even though they are both '2'. Using a constant for the column might make more sense. The patch I'm uploading uses a variable for now to at least give '2' a meaning.
general strategy
dawehner recommended taking a look at not adding the column in the first place. And I think it would remove some of the awkwardness of later removing the "2th" column. So we should take a look at not adding the column instead of removing it.
tests needed
this also should have tests written.
test case a: hide it! 3 languages enabled, content just exists (table would have all rows with source = n/a
test case b: hide it! 3 languages enabled, content exists and two translation exists both using the same source
test case c: show it! 3 languages enabled, content exists and two translation exists each using a different source
rare weirdness that can be ignored
[we can ignore this because "it's not a common/sane workflow that translators would do": There is a very raw use case where content is created in Spanish, an english translation is added (using spanish as the source), then the content is edited and the language of it is changed to be german. That would result in a table with a german row (the content) and a english row (english translation). the source of the english translation is/was spanish, but the source column would be hidden.]
Comment #18
YesCT CreditAttribution: YesCT commentedneeds tests
Comment #19
YesCT CreditAttribution: YesCT commentedComment #20
YesCT CreditAttribution: YesCT commentedcorrected my spelling of lauguage to language.
Now it only adds the column if there is more than one unique source language.
This did clean up the need to explain the 2th column or that n/a plus a source meant a comparison against '2'.
relocates the 2 lines setting up the header and rows arrays until after we know if we want to show the source column or not.
It does mean an extra foreach loop through all the languages enabled on a site.
(still needs tests)
Comment #21
YesCT CreditAttribution: YesCT commentedunassigning myself.
Also: Enzo, thanks for starting on this, without your work I could not have gotten this far. Maybe since you are familiar with the issue, you could also give my approach a review? :)
Comment #22
YesCT CreditAttribution: YesCT commentedah, $count_language_as_source is not used...
I should have changed that to $language_as_source and if I had, then I probably would not have had to do the isset($language_as_source)
Comment #23
shnark CreditAttribution: shnark commentedhere are some instructions for testing the patch
clean drupal 8.
download patch
made new branch
applied patch
installed drupal 8
under extend enable entity translation
under strutre edit article content type
under language settings enable translation
edit aricle content type
go to manage fields
edit body
enabled translation
under configuation add a language
add article pice of content
fill out words for title and body
click on tranlations tab
notice there is no source column
add translation
notice there is no source column
add another language
add another translation, picking a different source
notice there is a source column
Comment #24
klonos...coming a bit late in this, but what do you think about adding a "(source language)" or plain "(source)" text appended as some kind of indication when the column is hidden? I know that we already mark the language with bold text, but it kinda feels that it's not that obvious to people not familiar with this.
Comment #25
YesCT CreditAttribution: YesCT commented@klonos yeah, there is an issue for that: #1833126: Having the original language in bold text in the node "TRANSLATIONS" tab is not semantic. make representation accessible. which also links to an issue to figure out what to call it "source" etc...
Comment #26
shnark CreditAttribution: shnark commentedI made the change recommended in comment #22
since the if condition has a isset, we didn't need the line at the beginning.
besides, it had the wrong variable name at the beginning.
Comment #27
klonos@YesCT: Thanx for the pointer Cathy ;) Heading there...
Comment #28
YesCT CreditAttribution: YesCT commented#26: drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch queued for re-testing.
Comment #30
shnark CreditAttribution: shnark commented#26: drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch queued for re-testing.
Comment #30.0
shnark CreditAttribution: shnark commentedUpdated issue summary to add link to screenshots
Comment #31
YesCT CreditAttribution: YesCT commentedNext steps here include writing tests. For a hint for someone new to tests... would web tests be needed here (instead of unit tests)?
Comment #32
docker CreditAttribution: docker commentedTested as described #23
The source translation column only shown when there is more than n/a and the original language.
Comment #33
YesCT CreditAttribution: YesCT commented#26: drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch queued for re-testing.
Comment #34
YesCT CreditAttribution: YesCT commentedstill applies and passes testbot.
needs work to write tests. (Contributor Task for tests: http://drupal.org/node/1468170 )
Comment #35
nagwani CreditAttribution: nagwani commentedSubscribing to write tests
Comment #36
rli#26: drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch queued for re-testing.
Comment #37
PinoloComment #38
Pinolo-
Comment #39
PinoloPatch updated (no longer worked with latest core 8.x: column was always hidden since the source property was moved as element of translation[$langcode]). Still needs tests.
Comment #40
saltednutAfter applying the patch, during a quick manual test, I am only seeing the source translation column if there are more than 2 source languages.
Single translation (no source column)
Multiple translations with single source (no source column)
Multiple translations with multiple sources (source column)
Comment #41
PinoloBrantwynn: it's actually visible when 2 or more source languages are present. I understand this was the desired behavior. Can anyone confirm?
Comment #42
plachYes, this is the desired behavior: not cluttering the translation overview with the source column unless there actually is meaningful data to display.
Code looks good to me, aside from:
Can we have just
$show_shource_column = count($language_as_source) > 1;
and initialize$show_shource_column
to an empty array before the loop above?We should also add some test coverage for this, it shouldn't be a big deal.
Comment #43
saltednut@Pinolo - sorry for not explaining further but basically I was saying in #40 that your patch worked - I wasn't ready to RTBC it though because my time at the sprint was ending and I didn't have time to do code review.
So, by "multiple" I meant "2 or more" - sorry if that was lost in translation. :)
Comment #44
Gábor HojtsyComment #45
star-szrCNW for tests. Needs a reroll first, the file doesn't exist anymore.
Comment #46
dlu CreditAttribution: dlu commentedThis is a reroll of #40 without the change suggested in #42.
Comment #47
dlu CreditAttribution: dlu commentedMade change suggested in #42 assuming that the array to be initialized was really $language_as_source.
Comment #48
dlu CreditAttribution: dlu commentedMade change suggested in #42 assuming that the array to be initialized was really $language_as_source.
Comment #49
dlu CreditAttribution: dlu commentedHoping that this will make the test bot interested…
Comment #51
dlu CreditAttribution: dlu commentedTesting failed due to a regression from $language->id back to $language->langcode introduced by the reroll.
Comment #53
dlu CreditAttribution: dlu commentedTry it again without the typo…
Comment #54
dlu CreditAttribution: dlu commentedIt's late.
Comment #55
CMS Dude CreditAttribution: CMS Dude commentedTesting: Walked through steps as outlined in #23
- Applied patch in #53
- Fresh drupal 8 install
- Created content
- Added a few new languages
- No source language column showing (as per expected)
- Created a few nodes and altered so we had more than one source language.
- "Source Language" Column showed up as expected. Woo-hoo!
Comment #56
CMS Dude CreditAttribution: CMS Dude commentedGoing to write my first test.
Comment #56.0
CMS Dude CreditAttribution: CMS Dude commentedUpdated issue summary remaining tasks.
Comment #57
willieseabrook CreditAttribution: willieseabrook commented53: drupal8.content-translation.module.1832870-53.patch queued for re-testing.
Comment #59
willieseabrook CreditAttribution: willieseabrook commentedStarting as part of DrupalSprintWeekend
Comment #60
willieseabrook CreditAttribution: willieseabrook commentedComment #61
seranooo5 CreditAttribution: seranooo5 commentedComment #62
seranooo5 CreditAttribution: seranooo5 commentedComment #65
ivanchaer CreditAttribution: ivanchaer commentedComment #66
ivanchaer CreditAttribution: ivanchaer commentedIt seems that @seranooo5 forgot to keep one line of code from the original in his patch, hence the syntax error.
I've gone through the test patch steps (#23), and it seems fine now.
Comment #68
robertdbailey CreditAttribution: robertdbailey commentedRerolled patch #66 from January.
fixed $header and $row variables to exist even when multilingual is not enabled.
Removed unused $links variable.
Please review
Comment #69
robertdbailey CreditAttribution: robertdbailey commentedComment #70
Gábor HojtsyThanks for working on this!
I think the code looks good.
The issue summary needs an update with before/after screenshots. Also the summary still says if there are 2 sources, they should still not be displayed, but that is not what the code does. It displays sources even if there are only 2 of them. So maybe not implementing the exact right thing yet?
Also still needs tests :)
Apart of that, only cosmetic remarks:
I think this could likely do with some comments removed :) Eg. the table header re-setup comment is misleading, it does not set up the table but the header, although its easy to tell from the code itself. Its not that complicated to need a comment :)
Also inside the foreach I think one line of comment would be enough, something like:
// Collect source languages for translations (non-originals).
Or something along those lines.
Comment #71
robertdbailey CreditAttribution: robertdbailey commentedIssue summary updated.
Comment #72
robertdbailey CreditAttribution: robertdbailey commentedComment #73
robertdbailey CreditAttribution: robertdbailey commentedModified the patch to consolidate a little code and some comments. Let me know if this looks good, and I'll take a stab at the tests.
Comment #74
robertdbailey CreditAttribution: robertdbailey commentedoops, overzealous line consolidation caused some errors, patch resubmitted.
Comment #76
mauzeh CreditAttribution: mauzeh commentedMy first novice issue, I'm at DDD@Szeget 2014, will write tests for this issue.
Comment #77
mauzeh CreditAttribution: mauzeh commentedI will write the tests on the UN-patched version that will:
1) Fail if content is not yet translated (in which case all "source language" values are equal to "N/A"). Column should be hidden in this case.
2) Fail if content is written in different languages from scratch (in which case all "source language" values are also equal to "N/A"). Column should be hidden in this case.
3) Fail if at most one content item is translated from a version in a different language (in which case all but one "source language" is equal to "N/A"). Column should be hidden in this case.
4) Succeed if more than one content item is translation from a version in a different language (in which case more than one "source language" is equal to "N/A"). Column should be shown in this case.
Comment #78
mauzeh CreditAttribution: mauzeh commentedTwo patches:
1) Tests only. Should fail.
2) Patch and tests. Should pass.
Comment #81
mauzeh CreditAttribution: mauzeh commentedSeems like the tests failed because other modules (like Custom Block) inherit from my newly added test. I just ran the test locally (having applied the patch and test) and been able to reproduce the problem by running the test of the "Custom Block" module only.
Comment #82
mauzeh CreditAttribution: mauzeh commentedFixed the test. The "TESTS-ONLY" file should fail, and the other patch should pass.
Comment #84
plachLooks good, thanks!
Comment #85
webchickHey, nice. This is a sweet little UX improvement to make the screen look less cluttered for the 80%+ use case.
Unfortunately, though, it no longer applies, so needs a quick re-roll.
Comment #86
mauzeh CreditAttribution: mauzeh commentedHa! good to exercise my reroll muscles... There was one conflict introduced in #2111887: Regression: Only title (of base fields) on nodes is marked as translatable. The conflict was one line in the git context lines, I adopted the HEAD version.
Comment #88
YesCT CreditAttribution: YesCT commentedTalked to @mauzeh Looked at a diff of the two patches. It is good. Was a simple reroll, and patch with fix and test is green. We should be good to go here again.
Comment #89
mauzeh CreditAttribution: mauzeh commentedComment #91
robertdbailey CreditAttribution: robertdbailey commented86: 1832870-86.patch queued for re-testing. Previous result:
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
Comment #92
filijonka CreditAttribution: filijonka commentedCan't see that anything happened since #88 and patch passed the test again so setting this to RTBC.
Comment #94
filijonka CreditAttribution: filijonka commentedHi
Ok didn't have anyone to ask about this but since the patch obviously wasn't appliable I made a reroll based on #86. There were no conflicts so it should be straight forward I hope.
Comment #96
filijonka CreditAttribution: filijonka commented94: 1832870-94.patch queued for re-testing.
Comment #97
YesCT CreditAttribution: YesCT commentedI agree, only a change in context line
< foreach (field_info_instances($entity->getEntityTypeId(), $entity->bundle()) as $instance) {
---
> foreach (\Drupal::entityManager()->getFieldDefinitions($entity->getEntityTypeId(), $entity->bundle()) as $field_definition) {
Comment #98
tstoecklerI think that code should be moved closer to each other, so that it's easier to see that they are mutually exclusive. I.e. an if/else or something like that.
I think this part is very hard to read.
A: I think the isset() check should be moved into the if (). The empty string thing feelds weird.
I think also there's something that doesn't work as expected as it seems i.e. if there's one translation that has 'English' as source and then one that doesn't have a source at all, that would still result in the source_column being shown as
count(array('' => ..., 'en' => ...)) == 2
. ?!B: I think instead of the current variable/array structure I would propose the following:
That has a performance offset incurred by the array_unique() but that's negligible.
Is there someway we can avoid looping the list of languages twice? I don't think the performance loss is problematic but still would be nice. It might be that that is not possible or at least not easily possible.
There's no reason for format_string() here as there's nothing to format. In case you do want to stick to this please use String::format()
Moving back to needs review, to see if people agree with the points above.
Comment #99
vijaycs85Adding sprint tag.
Comment #100
YesCT CreditAttribution: YesCT commentedmauzeh, unassigning, just comment if you begin any work. :)
certainly at least 4. is actionable.
lets give a day or so to try the other things, and if they are not easy, let's re-rtbc and have them be follow-ups to investigate doing them.
Comment #101
Gábor HojtsyAdd D8MI topic tag.
Comment #102
robertdbailey CreditAttribution: robertdbailey commentedThis patch is an attempt at @tstoeckler's suggested changes.
Hopefully the hard-to-read passage is a little easier now. One observation: the size of the array of source languages should be at least one, not including the original language. This distinction seems important because a source language could be the only one and also not the original language at the same time. This is how it would happen:
a. create the original in English.
b. translate from English to French.
c. translate from French to German.
d. delete the French.
In this case, I think it is still important to show the source-language column, which would contain "n/a", "n/a", and "French", respectively. To account for this scenario, I just kept the collection of source languages as an associative array instead of the array to be unique'd per @tstoeckler's suggestion, so the original language may be easily removed if it exists (in the filter function). If this is the correct logic, then the issue summary should be updated to reflect this change.
Comment #103
tstoecklerYay, thanks a lot @robertdbailey!! It might be because I like anonymous functions a lot, but I find the code a *lot* more readable now. Awesome!
I have a few minor suggestions and then I think we're done here:
We should avoid arbitrary variable names such as
$a
. It seems$translation might be appropriate here?
I think the first two conditions can be collapsed with
!empty($a['source'])
It seems this is also equivalent to a
!empty()
. In that case maybe collapse the two statements into one?If we were to do a $show_source_column = FALSE before the big if {} then we can remove the isset().
Comment #104
robertdbailey CreditAttribution: robertdbailey commentedgreat catches @tstoeckler, changes made! I also bumped into a case where $entity->translation was not set because the content type was not enabled for translation, so I added one additional "!empty($entity->translation)" to prevent a warning in this case. Everything else appeared to work. Please review.
Comment #107
robertdbailey CreditAttribution: robertdbailey commentedI think (PHP 5.3.x?) did not like the last edit, consolidating the two lines into one. Perhaps there is a better way to do that; but I kept them separate for now. Please review.
Comment #109
tstoecklerYay!!!
Thanks a lot @robertdbailey for enduring my nitpicking. :-) I really do find the code a lot more natural to read, though, now. I have nothing more to complain about. The tests-only patch beautifully fails on the correct assertions.
RTBC.
Comment #110
Gábor HojtsyAdd D8MI topic tag.
Comment #111
webchickOops. thought I had committed this a long time ago! Rectifying that now. :)
Committed and pushed to 8.x. Thanks!
Comment #113
Gábor HojtsyYay, thanks all! Finally :)