New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n causing regressions #2637

Closed
jpouellet opened this Issue Feb 15, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@jpouellet
Contributor

jpouellet commented Feb 15, 2017

Qubes-manager notifications have been broken since QubesOS/qubes-manager@4946d7b because .tr() is attempted to be called on self in places where self is not the QApplication (or anything deriving from QObject?). ([1], [2], [3], almost certainly others)

This results in stack traces like the following:

Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/lib64/python2.7/threading.py", line 804, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/site-packages/pyinotify.py", line 1505, in run
    self.loop()
  File "/usr/lib/python2.7/site-packages/pyinotify.py", line 1491, in loop
    self.process_events()
  File "/usr/lib/python2.7/site-packages/pyinotify.py", line 1287, in process_events
    self._default_proc_fun(revent)
  File "/usr/lib/python2.7/site-packages/pyinotify.py", line 924, in __call__
    return _ProcessEvent.__call__(self, event)
  File "/usr/lib/python2.7/site-packages/pyinotify.py", line 644, in __call__
    return meth(event)
  File "/usr/lib64/python2.7/site-packages/qubesmanager/main.py", line 105, in process_IN_CLOSE_WRITE
    trayIcon.showMessage(unicode(self.tr(
AttributeError: 'QubesManagerFileWatcher' object has no attribute 'tr'

I tried just s/self.tr(/app.tr(/ everywhere, but app isn't available everywhere either, so apparently we need to review the scopes on a call-by-call basis? I feel like there should be a cleaner way to address this, but I don't know what that would be.

@jpouellet jpouellet changed the title from i18n regressions to i18n causing regressions Feb 15, 2017

@andrewdavidwong andrewdavidwong added this to the Release 3.2 updates milestone Feb 16, 2017

@jpouellet

This comment has been minimized.

Show comment
Hide comment
@jpouellet

jpouellet Feb 17, 2017

Contributor

There are also more regressions where .tr() is callable, but still missing conversion from QString to something with .format(). QubesOS/qubes-manager@d073b35 did not hit all cases, for example here.

Definitely don't release to qubes-dom0-current yet ;)

Contributor

jpouellet commented Feb 17, 2017

There are also more regressions where .tr() is callable, but still missing conversion from QString to something with .format(). QubesOS/qubes-manager@d073b35 did not hit all cases, for example here.

Definitely don't release to qubes-dom0-current yet ;)

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Feb 19, 2017

Member

([1], [2], [3], almost certainly others)

Any other example? Most of the code is in QObject based classes - the only exceptions I remember are:

  • few classes at the beginning of main.py (you've hit this one)
  • block.py, clipboard.py - excluded from translation for this reason
  • backup_utils.py, appmenu_select.py - nothing to translate

As for wrapping self.tr with unicode - this is rather ugly, but mostly because QString is incompatible with python builtin str/unicode... The alternative is to switch to QString completely and use QString.arg (%1, %2, etc) instead of str.format ({} or named variant). IMHO consistency with our other python modules is more important than consistency with PyQt.

Member

marmarek commented Feb 19, 2017

([1], [2], [3], almost certainly others)

Any other example? Most of the code is in QObject based classes - the only exceptions I remember are:

  • few classes at the beginning of main.py (you've hit this one)
  • block.py, clipboard.py - excluded from translation for this reason
  • backup_utils.py, appmenu_select.py - nothing to translate

As for wrapping self.tr with unicode - this is rather ugly, but mostly because QString is incompatible with python builtin str/unicode... The alternative is to switch to QString completely and use QString.arg (%1, %2, etc) instead of str.format ({} or named variant). IMHO consistency with our other python modules is more important than consistency with PyQt.

@jpouellet

This comment has been minimized.

Show comment
Hide comment
@jpouellet

jpouellet Feb 19, 2017

Contributor

It seems I was wrong. All .tr()s except those three (and this) indeed do seem to operate on self in a class deriving from QSomething.

PR to correct just those 3: QubesOS/qubes-manager#26

Contributor

jpouellet commented Feb 19, 2017

It seems I was wrong. All .tr()s except those three (and this) indeed do seem to operate on self in a class deriving from QSomething.

PR to correct just those 3: QubesOS/qubes-manager#26

@jpouellet jpouellet referenced this issue in QubesOS/qubes-manager Feb 19, 2017

Merged

Use app instead of self for .tr() outside QObject #26

@marmarek marmarek closed this Feb 19, 2017

jpouellet added a commit to jpouellet/qubes-manager that referenced this issue Feb 20, 2017

@jpouellet jpouellet referenced this issue in QubesOS/qubes-manager Feb 20, 2017

Merged

Add more missing unicode()s around .tr()s #27

jpouellet added a commit to jpouellet/qubes-manager that referenced this issue Feb 20, 2017

@qubesos-bot

This comment has been minimized.

Show comment
Hide comment
@qubesos-bot

qubesos-bot Feb 20, 2017

Automated announcement from builder-github

The package qubes-manager-3.2.9-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

Automated announcement from builder-github

The package qubes-manager-3.2.9-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

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