Tatoeba / tatoeba2

Tatoeba is a platform whose purpose is to create a collaborative and open dataset of sentences and their translations.
https://tatoeba.org
GNU Affero General Public License v3.0
702 stars 132 forks source link

Clearer transcription tooltips #3104

Closed profesyonal closed 5 months ago

profesyonal commented 6 months ago

2352

profesyonal commented 6 months ago

I did test this BTW, but I was having problems with merging a commit without including some other irrelevant changes I'd made, so I did the edit on Github.

Yorwba commented 6 months ago

Thanks again! While testing, I noticed that the changes didn't affect the old design (which is supposed to go away at some point, but can still be selected by users in their settings if they prefer it). That's because some of this code is duplicated in two functions in src/View/Helper/TranscriptionsHelper.php.

You could replace the same strings in TranscriptionsHelper.php as well (easiest), or rewrite it to eliminate the duplication by using the transcription's info_message property, which you've already updated. (This second solution might be a bit more finicky.)

profesyonal commented 6 months ago

Thank you!

I went with the second solution, but now I'm having trouble putting it on Github. Getting this (except with line breaks):

[username@MACHINE tatoeba2]$ git push origin patch-3 error: src refspec patch-3 does not match any error: failed to push some refs to 'https://github.com/profesyonal/tatoeba2.git'

I couldn't find any helpful solutions to this error by googling.

Yorwba commented 6 months ago

First of all, you shouldn't have to use sudo for git. If you're getting a "permission denied" error about some local file in .git when doing some operations (e.g. error: insufficient permission for adding an object to repository database .git/objects), you may have to fix ownership of git's internal files by running sudo chown -R $USER: .git so that they're no longer owned by the root user.

Secondly, the git push origin patch-3 command means: "Take the commit at patch-3 in my local repository and push it to the patch-3 branch of the remote origin repository." The error message error: src refspec patch-3 does not match any means that at the source (your local repository) there's nothing under the name patch-3 (refspec basically means "a way to refer to commits", this includes branch names, commit hashes, stuff like HEAD etc.)

Most likely you do not have a patch-3 branch in your local repository. If you have named your local branch differently (or you have not created a branch yet, which you can do with git checkout -b transcription-tooltips-3104 for e.g.) you can push it with git push --set-upstream origin $YOUR_LOCAL_BRANCH:patch-3, which also remembers the mapping so that you can simply use git push afterwards.

profesyonal commented 6 months ago

I pulled patch-3 onto my machine, re-did the edit, did git add -p, did git push, did git commit, and the changes never showed up.

I saw something about merging a commit on the interface, so I tried that, thinking it was mine... and I ended up pulling all of Tatoeba dev into this pull request.

I don't know why things have to be so difficult.

profesyonal commented 6 months ago

I'm probably going to need to undo this pull request somehow and start a new one.

trang commented 6 months ago

I pulled patch-3 onto my machine, re-did the edit, did git add -p, did git push, did git commit, and the changes never showed up.

You need to do the commit before the push (add → commit → push).

If you did git add then git push right after, then there was no commit, so nothing was sent to GitHub.

I saw something about merging a commit on the interface, so I tried that, thinking it was mine... and I ended up pulling all of Tatoeba dev into this pull request.

You don't have to worry about this. It didn't affect the content of the pull request. Note that whenever you push new commits, you can see what are the effective changes by looking at the "Files changed" tab of the pull request.

image

profesyonal commented 6 months ago

Thanks! Let's see how this does.

Yorwba commented 5 months ago

Thank you for your changes @profesyonal !

I don't know whether you noticed, but our automated tests failed because some test was generating notices about Trying to get property 'info_message' of non-object.

This notice is output when you attempt to use a PHP array as if it was an object, i.e. $transcr->info_message instead of $transcr['info_message']. The test producing these notices is phpunit tests/TestCase/Controller/TranscriptionsControllerTest.php --filter testResetDoesResetTranscription.

You can also see them when doing a reset in the old design: Transcription reset

The underlying reason is that when the transcription is reset, it is sent back as a freshly-generated array instead of being fetched from the database, so you cannot use object property syntax with ->.

While investigating this, I discovered that there are also other places in the codebase where this edge case is not handled properly.

Since the necessary change to your code is fairly small, I don't want to ask you to make it and then wait another few weeks until I have time to look at it again, so I'll merge this PR as-is and then apply the fixes for this edge case separately.

Yorwba commented 5 months ago

This change is now live :tada: