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

Proper handling of UTF-8 content in _print_msg #233

Merged
merged 3 commits into from Mar 12, 2015

Conversation

Projects
None yet
2 participants
@moollaza
Member

moollaza commented Feb 28, 2015

Fixes #232

Before:

2015-02-27_20h35_50

After:

2015-02-27_20h58_58

/cc @zachthompson

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Feb 28, 2015

Member

Hmm actually this might not be right:

2015-02-27_21h02_41

I'm not sure if this a problem with Data::Printer, or our code though. The front end result is correct:

2015-02-27_21h12_59

Member

moollaza commented Feb 28, 2015

Hmm actually this might not be right:

2015-02-27_21h02_41

I'm not sure if this a problem with Data::Printer, or our code though. The front end result is correct:

2015-02-27_21h12_59

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Feb 28, 2015

Contributor

If I add

use open qw/:std :utf8/;

to App::DuckPAN::Web/Query, both of those work for me.

Contributor

zachthompson commented Feb 28, 2015

If I add

use open qw/:std :utf8/;

to App::DuckPAN::Web/Query, both of those work for me.

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Feb 28, 2015

Contributor

You can also just add it to App::DuckPAN, though it will involve more testing.

Contributor

zachthompson commented Feb 28, 2015

You can also just add it to App::DuckPAN, though it will involve more testing.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 3, 2015

Member

@zachthompson hmm it seems to work better for sure, but still some weird stuff happens for edge cases like braille Mönchengladbach -- that still spits out the broken text instead of the O (umlaut).

Also, it seems like use open needs to be added to App/DuckPAN.pm because otherwise the Braille IA throws an error that there's a wide character in print coming from _print_msg

Member

moollaza commented Mar 3, 2015

@zachthompson hmm it seems to work better for sure, but still some weird stuff happens for edge cases like braille Mönchengladbach -- that still spits out the broken text instead of the O (umlaut).

Also, it seems like use open needs to be added to App/DuckPAN.pm because otherwise the Braille IA throws an error that there's a wide character in print coming from _print_msg

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Mar 3, 2015

Contributor

@moollaza I don't see broken text for "braille Mönchengladbach". How can I reproduce? I take it you're running server, as I don't think query accepts utf8. If you add the use statement to both Query and Web, all of the filehandles passed to App::DuckPAN will be utf8.

Contributor

zachthompson commented Mar 3, 2015

@moollaza I don't see broken text for "braille Mönchengladbach". How can I reproduce? I take it you're running server, as I don't think query accepts utf8. If you add the use statement to both Query and Web, all of the filehandles passed to App::DuckPAN will be utf8.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 4, 2015

Member

@zachthompson sorry for the confusion, I was referring to DuckPAN Query -- DuckPAN Server works fine, and query no longer croaks, but I sometimes don't see characters rendered properly.

It's obviously not as important as the Server output, but I still use Query every now and then, so I was hoping I could get it to display UTF8 properly. I think it may also have to do with my terminal emulator and font so, there's clearly lots of variables at play.

@zachthompson if you're happy with the PR we should be good to merge then.

Member

moollaza commented Mar 4, 2015

@zachthompson sorry for the confusion, I was referring to DuckPAN Query -- DuckPAN Server works fine, and query no longer croaks, but I sometimes don't see characters rendered properly.

It's obviously not as important as the Server output, but I still use Query every now and then, so I was hoping I could get it to display UTF8 properly. I think it may also have to do with my terminal emulator and font so, there's clearly lots of variables at play.

@zachthompson if you're happy with the PR we should be good to merge then.

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Mar 4, 2015

Contributor

Might want to add the use to App::DuckPAN::Query for Data::Printer. Does A/D/C/Server.pm really need the use for the progress bar? On a side note, I don't think A::D::C::Server is using Data::Printer.

Contributor

zachthompson commented Mar 4, 2015

Might want to add the use to App::DuckPAN::Query for Data::Printer. Does A/D/C/Server.pm really need the use for the progress bar? On a side note, I don't think A::D::C::Server is using Data::Printer.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 4, 2015

Member

@zachthompson whoops, meant to add it to Query.pm, not Server.pm! (Web.pm covers Server's use of Data::Printer)

Does A/D/C/Server.pm really need the use for the progress bar?

Yup, it uses the progress bar on initialization when it downloads the various files it requires from DDG.

For those of us on faster internet connections you barely notice it, but a previous dev had a really slow connection and was running into problems so he wanted to be able to check if things were completing and I guess how much longer they were going to take.

Member

moollaza commented Mar 4, 2015

@zachthompson whoops, meant to add it to Query.pm, not Server.pm! (Web.pm covers Server's use of Data::Printer)

Does A/D/C/Server.pm really need the use for the progress bar?

Yup, it uses the progress bar on initialization when it downloads the various files it requires from DDG.

For those of us on faster internet connections you barely notice it, but a previous dev had a really slow connection and was running into problems so he wanted to be able to check if things were completing and I guess how much longer they were going to take.

@moollaza

This comment has been minimized.

Show comment
Hide comment
@moollaza

moollaza Mar 4, 2015

Member

@zachthompson should be good now!

Member

moollaza commented Mar 4, 2015

@zachthompson should be good now!

@zachthompson

This comment has been minimized.

Show comment
Hide comment
@zachthompson

zachthompson Mar 4, 2015

Contributor

I just meant does the progress bar need the "use open" as it's the only filehandle in the module. But it looks like you fixed that. Looks good to me.

Contributor

zachthompson commented Mar 4, 2015

I just meant does the progress bar need the "use open" as it's the only filehandle in the module. But it looks like you fixed that. Looks good to me.

moollaza added a commit that referenced this pull request Mar 12, 2015

Merge pull request #233 from duckduckgo/zaahir/utf8-fix
Proper handling of UTF-8 content in _print_msg

@moollaza moollaza merged commit 8cd1030 into master Mar 12, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@moollaza moollaza deleted the zaahir/utf8-fix branch Mar 12, 2015

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