Review Request 130224: fixes bug "Relative path for audio files not working"

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Review Request 130224: fixes bug "Relative path for audio files not working"

René Fritz
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130224/

Review request for KDE Edu.
By René Fritz.
Bugs: 381349
Repository: libkeduvocdocument

Description

description is in bug report https://bugs.kde.org/show_bug.cgi?id=381349

Testing

urls like "file:somepath/file.mp3" are parsed correctly again and sound is played

Diffs

  • keduvocdocument/readerwriters/keduvockvtml2reader.cpp (d5322f51266da66247248776430efa0cb7f98fdc)

View Diff

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request 130224: fixes bug "Relative path for audio files not working"

Albert Astals Cid-3
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130224/

Looks good, i'm going to go an extra step and ask.

Do you think you could either add an autotest (or improve one of the existing ones) to check this so we don't regress again?

If you think it's too much or need help, just say it :)


- Albert Astals Cid


On August 12th, 2017, 8:20 p.m. UTC, René Fritz wrote:

Review request for KDE Edu.
By René Fritz.

Updated Aug. 12, 2017, 8:20 p.m.

Bugs: 381349
Repository: libkeduvocdocument

Description

description is in bug report https://bugs.kde.org/show_bug.cgi?id=381349

Testing

urls like "file:somepath/file.mp3" are parsed correctly again and sound is played

Diffs

  • keduvocdocument/readerwriters/keduvockvtml2reader.cpp (d5322f51266da66247248776430efa0cb7f98fdc)

View Diff

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request 130224: fixes bug "Relative path for audio files not working"

René Fritz
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130224/

On August 13th, 2017, 3:42 p.m. UTC, Albert Astals Cid wrote:

Looks good, i'm going to go an extra step and ask.

Do you think you could either add an autotest (or improve one of the existing ones) to check this so we don't regress again?

If you think it's too much or need help, just say it :)

Hi, Good point I'm a developer but totally new to c++, qt, ... So writing a test would be a bit too much for now, maybe?

I don't see how keduvockvtml2reader.cpp could be tested easily. Too much context I guess?

So testing to read a whole document might be the way to go. I looked into the existing tests. Kvtml2ReaderTest would be the right place I think and creating test entries in the fixture xml is something I could do I guess. But testing the parsed document would mean I understand the KVOCREADER_EXPECT_CORE macro to get access to the parsed document ... which is not the case :-) A test example which takes XMLGenerator gen; and makes KEduVocDocument doc; out of it would be something I might be able to work with.


- René


On August 12th, 2017, 8:20 p.m. UTC, René Fritz wrote:

Review request for KDE Edu.
By René Fritz.

Updated Aug. 12, 2017, 8:20 p.m.

Bugs: 381349
Repository: libkeduvocdocument

Description

description is in bug report https://bugs.kde.org/show_bug.cgi?id=381349

Testing

urls like "file:somepath/file.mp3" are parsed correctly again and sound is played

Diffs

  • keduvocdocument/readerwriters/keduvockvtml2reader.cpp (d5322f51266da66247248776430efa0cb7f98fdc)

View Diff

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request 130224: fixes bug "Relative path for audio files not working"

Albert Astals Cid-3
In reply to this post by Albert Astals Cid-3
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130224/

On August 13th, 2017, 3:42 p.m. UTC, Albert Astals Cid wrote:

Looks good, i'm going to go an extra step and ask.

Do you think you could either add an autotest (or improve one of the existing ones) to check this so we don't regress again?

If you think it's too much or need help, just say it :)

On August 13th, 2017, 8:58 p.m. UTC, René Fritz wrote:

Hi, Good point I'm a developer but totally new to c++, qt, ... So writing a test would be a bit too much for now, maybe?

I don't see how keduvockvtml2reader.cpp could be tested easily. Too much context I guess?

So testing to read a whole document might be the way to go. I looked into the existing tests. Kvtml2ReaderTest would be the right place I think and creating test entries in the fixture xml is something I could do I guess. But testing the parsed document would mean I understand the KVOCREADER_EXPECT_CORE macro to get access to the parsed document ... which is not the case :-) A test example which takes XMLGenerator gen; and makes KEduVocDocument doc; out of it would be something I might be able to work with.

You don't really need KVOCREADER_EXPECT_CORE, the only thing that macro does is read the document and check it parsed successfully. You can't have access to the KEduVocDocument it contains.

Just create a new test_somethingSomething() function in Kvtml2ReaderTest and there declare all variables you need and QCOMPARE/QVERIFY in it


- Albert


On August 12th, 2017, 8:20 p.m. UTC, René Fritz wrote:

Review request for KDE Edu.
By René Fritz.

Updated Aug. 12, 2017, 8:20 p.m.

Bugs: 381349
Repository: libkeduvocdocument

Description

description is in bug report https://bugs.kde.org/show_bug.cgi?id=381349

Testing

urls like "file:somepath/file.mp3" are parsed correctly again and sound is played

Diffs

  • keduvocdocument/readerwriters/keduvockvtml2reader.cpp (d5322f51266da66247248776430efa0cb7f98fdc)

View Diff

Loading...