Re: [Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread phill
Hi Sam, Thinking about it further, this would be the wrong place to strip invalid chars such as tabs. Ideally this should be done by the code that commits the data to the database. This is common to the importers, as well as the song edit form and the reindexer. If your code works fine, I have no

Re: [Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread Samuel Findlay
You're right Phill, the non-printing bytes do specify the length of the following field/label. A rewrite based on this info should be more efficient. This may also avoid the need for stripping ascii control characters. On May 1, 2012 1:13 AM, "phill" wrote: > One more thing, lines 223 - 229: the

Re: [Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread Samuel Findlay
Though still makes sense to replace tab with space, as you suggest. On May 1, 2012 7:15 AM, "Samuel Findlay" wrote: > You're right Phill, the non-printing bytes do specify the length of the > following field/label. A rewrite based on this info should be more > efficient. > > This may also avoid t

Re: [Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread phill
One more thing, lines 223 - 229: the "non-printing byte" specifies how many chars (or bytes) long the "label" is. It is not unique to the label, i.e. If you had two labels the same length, the would have the same non-printing byte ay the start. Phill -- https://code.launchpad.net/~sfindlay/openl

Re: [Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread phill
Nice work, I like byte puzzles ;-) . I cant test your code atm, but I have a few comments below. Line 231: > +The field label is separated from the field contents by one random byte. That byte isn't random! It actually specifies how long the string is in characters (or bytes!) Iirc, the

[Openlp-core] [Merge] lp:~sfindlay/openlp/songs-import-powersong into lp:openlp

2012-04-30 Thread Samuel Findlay
Samuel Findlay has proposed merging lp:~sfindlay/openlp/songs-import-powersong into lp:openlp. Requested reviews: OpenLP Core (openlp-core) For more details, see: https://code.launchpad.net/~sfindlay/openlp/songs-import-powersong/+merge/104102 Added PowerSong song importer. * PowerSong is ope

Re: [Openlp-core] [Merge] lp:~googol/openlp/trivial into lp:openlp

2012-04-30 Thread Tim Bentley
Review: Needs Information Receiver.send_message(u'openlp_phonon_creation') removal needs to be considered. Cannot remember why this was added but it was so what is the impact of it's loss. -- https://code.launchpad.net/~googol/openlp/trivial/+merge/103988 Your team OpenLP Core is subscribed t