Hi Paul,
wow, thanks for your detailed review, that's verry much appreciated!
On 28.02.2016 at 15:53, Paul Wise wrote:
> On Sat, Feb 27, 2016 at 8:16 PM, Philip Rinn wrote:
>
>> Both points are addressed now and updated packaging is uploaded:
>> http://mentors.debian.net/package/gtranscribe
>
> I had a play with the app and love the simplicity, very useful for
> transcription of single-speaker audio.
Cool, that's good to hear.
> I have done a review of the package and am willing to upload if these
> two issues are fixed:
>
> The play/pause button doesn't appear to work for me.
>
> I can fairly consistently get it to segfault in Python using this
> sequence of events: open a short ogg file, press play, press faster,
> crash.
Sorry, I can't reproduce those bugs. It works for me - but as Giulio sees the same
bugs I'll set up a fresh installation to test. Do you use unstable?
> If you want to fix some other things at the same time:
>
> It isn't clear to me what the resume interval is for, does it resume
> playing after you press pause for a catch-up break?
It's the time the playback jumpf back when you resume. So if you pause at 12:14.3
and "resume intervall" is set to 1000 ms the playback will start again at 12:13.3.
> I think it would be useful to have speeds less than 0.5 for the slower
> typists amongst us.
True, I plan to make this configurable in the next release.
> Starting the app prints this on the terminal:
>
> (gtranscribe:24209): Gtk-CRITICAL **: gtk_widget_grab_default:
> assertion 'gtk_widget_get_can_default (widget)' failed
The only hint I found about it is
http://stackoverflow.com/questions/17058854/how-to-resolve-this-error-in-gtk-file-chooser-dialog
but I don't know how to do that :(
> Typing two spaces in a row prints this on the terminal:
>
> ** (gtranscribe:27472): CRITICAL **: enchant_dict_check: assertion 'len' failed
I think it's a bug in GtkSpell:
https://sourceforge.net/p/gtkspell/mailman/message/33494688/
> With no audio loaded, pressing play/forward/rewind prints this on the terminal:
>
> Traceback (most recent call last):
> File "/usr/bin/gtranscribe", line 286, in play_loop
> self.set_position_label(duration)
> File "/usr/bin/gtranscribe", line 293, in set_position_label
> frac = float(self.position) / float(duration)
> ZeroDivisionError: float division by zero
Fixed.
> With no audio loaded, pressing slower/faster prints this on the terminal:
>
> Traceback (most recent call last):
> File "/usr/bin/gtranscribe", line 331, in on_scale_speed_value_changed
> fileinfo = FileInfo(self.player.filename, self.md5)
> AttributeError: 'gTranscribeWindow' object has no attribute 'md5'
Fixed.
> With no audio loaded, pressing pause prints this on the terminal:
>
> Traceback (most recent call last):
> File "/usr/bin/gtranscribe", line 263, in play
> self._set_update_ui(False)
> File "/usr/bin/gtranscribe", line 150, in _set_update_ui
> self.play_loop()
> File "/usr/bin/gtranscribe", line 286, in play_loop
> self.set_position_label(duration)
> File "/usr/bin/gtranscribe", line 293, in set_position_label
> frac = float(self.position) / float(duration)
> ZeroDivisionError: float division by zero
Fixed.
> Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata
I don't understand why this is not generated from the AppStream metadata I just added.
> You might want to add some AppStream metadata: https://wiki.debian.org/AppStream
Done. (And already failed - forgot to update
> It would be great if you could sign your upstream releases with OpenPG
> and add the needed things to have uscan automatically verify upstream
> tarballs.
>
> https://wiki.debian.org/debian/watch#Cryptographic_signature_verification
>
> Some best practices for OpenPGP you might want to read:
>
> https://help.riseup.net/en/security/message-security/openpgp/best-practices
Sure, that's a good idea. To sign the releases on GitHub I followed
https://wiki.debian.org/Creating%20signed%20GitHub%20releases
unfortunately it didn't work. When uploading the signature file I just get an
error :( I'll try some more but will not do it for this release.
> Your debian/rules seems overly complex, are all the overrides needed?
Well, I didn't manage to get a python3 only build working without - I adopted it
from http://sources.debian.net/src/onboard/1.1.2-2/debian/rules/
> You might want to use wrap-and-sort to make diffs of the Debian
> packaging easier to read. My favourite command-line for this is:
>
> wrap-and-sort --short-indent --wrap-always --sort-binary-packages
> --trailing-comma
Nice tool :). Fixed.
> Automated checks:
>
> build:
>
> WARNING: the following files are not recognized by DistUtilsExtra.auto:
> gpl-3.0.txt
> lgpl-2.1.txt
True, I'll fix this later, I don't need them for the Debian packaging.
> lintian:
>
> P: gtranscribe source: debian-watch-may-check-gpg-signature
> P: gtranscribe: no-upstream-changelog
To be done.
> check-all-the-things:
>
> $ cme check dpkg
> Warning in 'control source Build-Depends:4' value
> 'python3-distutils-extra (>= 2.22)': unnecessary versioned dependency:
> python3-distutils-extra >= 2.22. Debian has wheezy -> 2.36-1;
> jessie-kfreebsd -> 2.38-1; jessie -> 2.38-1; stretch -> 2.39-1; sid ->
> 2.39-1;
Fixed.
> $ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec POFileSpell {} +
> <po-file-spell>
> <file name="./po/de.po">
> ...
I don't understand the output, it just says:
> ...
> <error>Fehler: No word lists can be found for the language "de_DE".</error>
> ...
> $ find -type f \( -iname '*.po' -o -iname '*.pot' -o -iname '*.mo' -o
> -iname '*.gmo' \) -exec i18nspector {} +
> W: ./po/de.po: language-disparity de (pathname) != de_DE (Language header field)
> W: ./po/de.po: invalid-date PO-Revision-Date: (empty string)
> W: ./po/de.po: no-report-msgid-bugs-to-header-field
Fixed.
> $ license-reconcile
> ...
> Copyright mismatch: File gtranscribe/helpers.py: For copyright holder
> 'Philip Rinn <rinni@inventati.org>' the years 2013-2016 cannot be
> fitted into 2013-2014.
> Copyright mismatch: File gtranscribe/fileinfo.py: For copyright holder
> 'Philip Rinn <rinni@inventati.org>' the years 2013-2016 cannot be
> fitted into 2013-2014.
> ...
> Copyright mismatch: File gtranscribe/AboutDialog.py: For copyright
> holder 'Philip Rinn <rinni@inventati.org>' the years 2013-2016 cannot
> be fitted into 2013-2014.
Fixed.
> $ find -type f -iname '*.py' -exec pep8 --ignore W191 {} +
> ./gtranscribe/helpers.py:4:2: W291 trailing whitespace
> ./gtranscribe/helpers.py:6:68: W291 trailing whitespace
> ./gtranscribe/helpers.py:8:2: W291 trailing whitespace
> ./gtranscribe/helpers.py:13:2: W291 trailing whitespace
> ./gtranscribe/helpers.py:29:80: E501 line too long (101 > 79 characters)
> ./gtranscribe/helpers.py:34:1: E302 expected 2 blank lines, found 1
> ./gtranscribe/helpers.py:41:1: W293 blank line contains whitespace
> ...
Mostly fixed.
> $ find -type f -iname '*.py' -exec pyflakes {} +
> ./gtranscribe/helpers.py:23: 'logging' imported but unused
>
> $ find -type f -iname '*.py' -exec pyflakes3 {} +
> ./gtranscribe/helpers.py:23: 'logging' imported but unused
Fixed.
> $ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname
> .svn -o -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o
> -iname _darcs -o -iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o
> -iname .metadata -o -iname CMakeFiles -o -iname _build -o -iname
> _sgbak -o -iname autom4te.cache -o -iname blib -o -iname cover_db -o
> -iname node_modules -o -iname '~.dep' -o -iname '~.dot' -o -iname
> '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( -iname '*.bak' -o
> -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 'core.*' -o
> -iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o
> -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname
> '*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname
> '*.css.min' \) -exec spellintian --picky {} +
> ./lgpl-2.1.txt: GNU Library Public License -> GNU Library General Public License
> ./data/ui/gTranscribe.glade: gtk+ -> GTK+
Fixed.
> $ grep -riE 'fixme|todo|hack|xxx' .
> ./bin/gtranscribe: # TODO: Make these configurable
True, but not done jet :(.
I updated the packaging, you can find it at:
https://anonscm.debian.org/cgit/collab-maint/gtranscribe.git
http://mentors.debian.net/package/gtranscribe
Best,
Philip
Attachment:
signature.asc
Description: OpenPGP digital signature