wxHtmlPrintout i18n and DIP fixes (PR #24916)
Blake-Madden
Make default titles translatable, don't piece strings together for title
Also, make initial frame size DPI aware
Remove wxT() macros
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24916
Commit Summary
- 6215c51 Make default titles translatable, don't piece strings together for title
File Changes
(2 files)
- M include/wx/html/htmprint.h (4)
- M src/html/htmprint.cpp (25)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24916.patch
- https://github.com/wxWidgets/wxWidgets/pull/24916.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, I'll merge it with the changes proposed below if you don't mind.
>
- num.Printf(wxT("%lu"), (unsigned long)(m_PageBreaks.size() - 1));
- r.Replace(wxT("@PAGESCNT@"), num);
+ num.Printf("%lu", (unsigned long)(m_PageBreaks.size() - 1));
As long as we're modifying this, we should make it
⬇️ Suggested change- num.Printf("%lu", (unsigned long)(m_PageBreaks.size() - 1));
+ num.Printf("%zu", m_PageBreaks.size() - 1);
> + m_ParentWindow ? + m_ParentWindow->FromDIP(wxSize(650, 500)) : + wxSize(650, 500));
This can be written more simply as
⬇️ Suggested change- m_ParentWindow ? - m_ParentWindow->FromDIP(wxSize(650, 500)) : - wxSize(650, 500)); + wxWindow::FromDIP(wxSize(650, 500), m_ParentWindow));
> @@ -709,8 +709,11 @@ bool wxHtmlEasyPrinting::DoPreview(wxHtmlPrintout *printout1, wxHtmlPrintout *pr
}
wxPreviewFrame *frame = new wxPreviewFrame(preview, m_ParentWindow,
- m_Name + _(" Preview"),
- wxPoint(100, 100), wxSize(650, 500));
+ wxString::Format(_("%s Preview"), m_Name),
+ wxPoint(100, 100),
Probably should use wxDefaultPosition here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 26bb353 Use size_t format specifier
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- e380d7d Use static FromDIP
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -709,8 +709,11 @@ bool wxHtmlEasyPrinting::DoPreview(wxHtmlPrintout *printout1, wxHtmlPrintout *pr
}
wxPreviewFrame *frame = new wxPreviewFrame(preview, m_ParentWindow,
- m_Name + _(" Preview"),
- wxPoint(100, 100), wxSize(650, 500));
+ wxString::Format(_("%s Preview"), m_Name),
+ wxPoint(100, 100),
I thought that too when I saw it centering the window in the next line, but left it alone for fear of appearing too nit-picky :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- e407239 Use wxDefaultPosition
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi commented on this pull request.
In include/wx/html/htmprint.h:
> @@ -231,7 +231,7 @@ class WXDLLIMPEXP_HTML wxHtmlPrintout : public wxPrintout
class WXDLLIMPEXP_HTML wxHtmlEasyPrinting : public wxObject
{
public:
- wxHtmlEasyPrinting(const wxString& name = wxT("Printing"), wxWindow *parentWindow = nullptr);
+ wxHtmlEasyPrinting(const wxString& name = _("Printing"), wxWindow *parentWindow = nullptr);
As all of these are individual words marked as translatable, they could use a /* TRANSLATORS: ... */ comment to clarify what the string actually means, and in what context.
For instance, does "Printing" mean the same as "printout" (as a noun), or does it mean "now printing a document".
> @@ -709,8 +709,9 @@ bool wxHtmlEasyPrinting::DoPreview(wxHtmlPrintout *printout1, wxHtmlPrintout *pr
}
wxPreviewFrame *frame = new wxPreviewFrame(preview, m_ParentWindow,
- m_Name + _(" Preview"),
- wxPoint(100, 100), wxSize(650, 500));
+ wxString::Format(_("%s Preview"), m_Name),
Also here, a TRANSLATORS: comment could clarify that this related to rpinting, and that %s is a name (of a document?).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- e9eee4f Add wxGETTEXT_IN_CONTEXT to explain context of translatable strings
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden commented on this pull request.
> @@ -709,8 +709,9 @@ bool wxHtmlEasyPrinting::DoPreview(wxHtmlPrintout *printout1, wxHtmlPrintout *pr
}
wxPreviewFrame *frame = new wxPreviewFrame(preview, m_ParentWindow,
- m_Name + _(" Preview"),
- wxPoint(100, 100), wxSize(650, 500));
+ wxString::Format(_("%s Preview"), m_Name),
Got it. I added wxGETTEXT_IN_CONTEXT to all the strings. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 802da65 Use wxGetTranslation instead of wxGETTEXT_IN_CONTEXT
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
@lanurmi commented on this pull request.
I don't think this will work at all, for various reasons. Firstly, passing the formatted and localized "%s Preview" to wxGetTranslation() means a translation will virtually never be found for the argument. wxGetTranslation() (or _()) within wxGetTranslation() cannot be right.
Also the messages will not be properly picked with context by xgettext, if calling wxGetTranslation() with multiple arguments. wxGETTEXT_IN_CONTEXT` is what should be used when dealing with string literals.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- adbc6a4 Wrap strings in wxString to fix type ambiguity
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
wxGETTEXT_IN_CONTEXT is what should be used when dealing with string literals.
Some compilers are failing with wxGETTEXT_IN_CONTEXT, complaining about ambiguity between const char* and unsigned int. I was calling wxGETTEXT_IN_CONTEXT the same way it is called throughout the rest of the wx code base, so I have no idea what compiles in other places but not here.
So, for wxGETTEXT_IN_CONTEXT, what is the proper way to call it? With an 'L' in front of the literal string? Or wrap it in a wxString CTOR? Again, I tried simply calling wxGETTEXT_IN_CONTEXT with string literals like I see in many other places, but 4 or so build environments fail.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 94d10ba Use wxGETTEXT_IN_CONTEXT, wrap literal string in wxString CTOR to fix ambiguity compile errors
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 187e57a Try wide string literals with wxGETTEXT_IN_CONTEXT
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- f7866f2 Remove wxGETTEXT_IN_CONTEXT
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
I cannot get wxGETTEXT_IN_CONTEXT to compile with string literals in all build environments. I have tried wxString CTOR wrappers, L wide strings, char*, etc. Rolling back to simple _() calls.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
AppVeyor "failed" configuration says "All tests passed (1162787 assertions in 385 test cases)".
This seems to be good to go.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Lauri Nurmi
This seems to be good to go.
It is still entirely possible to add the /* TRANSLATORS: ... */ comments that I suggested earlier.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
This seems to be good to go.
It runs two tests, test.exe and test_gui.exe. It seems the second one got stuck and AppVeyor timed out after 1 hour. I think this sometimes randomly happens.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
@Blake-Madden pushed 1 commit.
- 0677b75 Add translator comments
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
Translator comments are in there now :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Blake-Madden
Failure is from unrelated "CHECK( m_browser->GetCurrentTitle() == "Title" )"
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.