Skip to content

Conversation

@b100dian
Copy link
Contributor

This should trigger transifex.

Also there were some strings marked as "vanished" in #53 - My guess is Qt Creator building (I built from commandline). I reverted only the translations/ part of that commit, added back what my commandline build generated, then added the de strings.

Next let's sett if transifex gets updated (it did, on my super secret org)

@nephros
Copy link
Contributor

nephros commented Sep 29, 2021

Also there were some strings marked as "vanished" in #53 - My guess is Qt Creator building (I built from commandline). I reverted only the translations/ part of that commit, added back what my commandline build generated, then added the de strings.

The"vanished" lines appear when lupdate is run, to mark previously translated strings for which the original source string is not found in the source files.

It is safe to keep them in there, usually things like Transifiex should be able to pick them up again should the source strings reappear some time later.

It's quite possible I mangled them locally when compiling, not sure.

@b100dian
Copy link
Contributor Author

b100dian commented Sep 29, 2021 via email

@b100dian
Copy link
Contributor Author

b100dian commented Oct 2, 2021

@Olf0 or @nephros - I still need to merge this "unvanishing" strings PR for two reasons:

  1. the vanished string are not translated anymore. They are not vanished however, pick one from the diff and see it's still there
  2. to see the transifex integration in action for the first time. This would better trigger the translation team.

I will probably follow up with updates like this whenever translations/settings-patchmanager.ts changes (you should include this one in PRs too) and will follow up with more roles on the transifex org, just need this test first. Thanks!

@Olf0
Copy link
Contributor

Olf0 commented Oct 3, 2021

@Olf0 or @nephros - I still need to merge this "unvanishing" strings PR ...

Sounds like you are indirectly asking for a review.
Well, I tried, but have to admit that I do not understand how the whole Transifex process works at all. Can you point me to some documentation?

  1. I also do not understand the addition of all the ../installroot/[...] paths, which can only occur while running rpmbuild, correct?
  2. I believe to understand that this pattern (also here) is just an abbreviated syntax, correct?
  3. I do not understand, why this block has to go.

Look, apart from shell scripts, spec files (the is only a single one around, here), history, some background knowledge, experience with PM and organisational stuff, my know-how is little to no help, as I originally announced. I can try to comprehend new stuff, though.
And these things do cost time, hence stalling my other little endeavours (searching, finding and submitting the olf-icon, setting up the Openrepos page, setting up the FSO-feedback page), as you also remarked.

Still I would like to understand the Transifex process a bit better, to comprehend how a commit containing enhanced source and target strings can be applied: PR #46
I.e., I could split that MR, but inputting all the enhanced target strings at Transifex manually is a no-go.
Also I do not like altering the source and target TS-files (which have been unchanged for years) before resolving and applying PR #46 (which also has been stalled for years). Can you please help with that, first? It is only string changes.

IMO, there is no reason to rush the topic translations, because there was a standstill for years!

@b100dian
Copy link
Contributor Author

b100dian commented Oct 3, 2021

Thanks for the review @Olf0 - I haven't noticed the installroot paths either, let me see if I can find their cause.

The changes are purely mechanical by lupdate, except "add back de strings" where I was adding back translations Peter already included.
The EN file doesn't seem to be used by the app or by transifex, that's why I didn't bother updating it. I will try to remove it (the non-suffixed file is EN)

@b100dian
Copy link
Contributor Author

b100dian commented Oct 3, 2021

The installroot folder is created by my way of building patchmanager, through a porters script which calls mb2 with CWD set to patchmanager and that creates installroot.

I haven't experience the literal asterisk though - but have not been able to generate .ts files update from Qt Creator, only through the workflow above, which duplicates some file references in installroot

@nephros
Copy link
Contributor

nephros commented Oct 3, 2021

I haven't experience the literal asterisk though - but have not been able to generate .ts files update from Qt Creator, only through the workflow above, which duplicates some file references in installroot

That might be related, from my very brief research about wildcards in pro files there was some mention using them confuses qtcreator somehow.

@b100dian
Copy link
Contributor Author

b100dian commented Oct 3, 2021

Ok, now I also know where the <name></name> vs <name/> comes from.
lupdate on my system is 5.15
mb2 build-shell lupdate is 5.6
The latter was contracting to <name/>

Sounds like manually running lupdate -pro is the way to go?

@Olf0
Copy link
Contributor

Olf0 commented Oct 3, 2021

Ok, now I also know where the <name></name> vs <name/> comes from. lupdate on my system is 5.15 mb2 build-shell lupdate is 5.6 The latter was contracting to <name/>

Sounds like manually running lupdate -pro is the way to go?

I did not say that this is wrong, only that this is a unexpected change, in contrast what you stated to be altered.

b100dian and others added 2 commits October 3, 2021 22:39
previously running a `lupdate -pro patchmanager.pro` created a file called `translations/settings-parchmanager-*.ts` (a literal asterisk)
using the `$$file` function seems to work much better
@b100dian
Copy link
Contributor Author

b100dian commented Oct 3, 2021

heads up, I'm reworking this using lupdate/lrelease commands over @nephros's commit

@b100dian
Copy link
Contributor Author

b100dian commented Oct 3, 2021

the two commits where no human edit was made have the commands attached

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

@b100dian
Copy link
Contributor Author

b100dian commented Oct 4, 2021

Thanks Olf for this pass. The vanished you're mentioning seem to be because 'Have possible conflicts' is now 'May have conflicts' and 'Developer mode' is also changed into "Allow incompatible patches".
I'd merge this and will observe tomorrow if the changes (esp. the DE ones) are picked up.

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

O.K., there you go.
But now we are really cheating with my "approval"!

@Olf0
Copy link
Contributor

Olf0 commented Oct 4, 2021

Please also mind that #73 alters more strings, but also simplifies some!
And it is too complicated (code-wise) that I will approve it.
Maybe it would be better to review #73 first and let @nephros merge it, before continuing with this one?

Either way: Please review #73!

@b100dian
Copy link
Contributor Author

b100dian commented Oct 5, 2021

Yes, I'll review that PR tonight.
I am going to merge this to see actual transifex in action, then we can iterate (make one more PR).
Remember, transifex only looks for changes in the translations/settings-patchmanager.ts file as it is configured now (and generates the other languages out of it - let's see!)
I have also added you as Project Lead on transifex. I didn't find a username @nephros to add Peter though.

But now we are really cheating with my "approval"!

No, I think it's part of the learning process, we must more than one person looking for the transifex integration. Consider it part of onboarding - even if this doesn't work as intended, learning it will be either way.

@b100dian b100dian merged commit d526041 into sailfishos-patches:master Oct 5, 2021
@Olf0
Copy link
Contributor

Olf0 commented Oct 5, 2021

No, I think it's part of the learning process, we must more than one person looking for the transifex integration.

Well, ack.
Though I am not convinced that I am appropriate for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants