-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
New text tool #4383
base: beta
Are you sure you want to change the base?
New text tool #4383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
} | ||
|
||
std::string faceValue() const { | ||
return m_face; | ||
FontInfo fontInfo() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: method 'fontInfo' can be made static [readability-convert-member-functions-to-static]
FontInfo fontInfo() const { | |
static FontInfo fontInfo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bug in clang-tidy.
src/app/ui/font_entry.cpp
Outdated
case kFocusEnterMessage: | ||
if (!m_popup) { | ||
try { | ||
FontInfo info = static_cast<FontEntry*>(parent())->info(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'info' of type 'FontInfo' can be declared 'const' [misc-const-correctness]
FontInfo info = static_cast<FontEntry*>(parent())->info(); | |
FontInfo const info = static_cast<FontEntry*>(parent())->info(); |
font->setSize(fontInfo.size()); | ||
} | ||
else { | ||
const int size = (fontInfo.useDefaultSize() ? 18: fontInfo.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
const int size = (fontInfo.useDefaultSize() ? 18: fontInfo.size());
^
src/app/util/render_text.cpp
Outdated
if (bounds.w < 1) bounds.w = 1; | ||
if (bounds.h < 1) bounds.h = 1; | ||
|
||
image.reset(doc::Image::create(doc::IMAGE_RGB, bounds.w, bounds.h)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
image.reset(doc::Image::create(doc::IMAGE_RGB, bounds.w, bounds.h));
^
src/app/util/render_text.cpp
Outdated
if (bounds.w < 1) bounds.w = 1; | ||
if (bounds.h < 1) bounds.h = 1; | ||
|
||
image.reset(doc::Image::create(doc::IMAGE_RGB, bounds.w, bounds.h)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
image.reset(doc::Image::create(doc::IMAGE_RGB, bounds.w, bounds.h));
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
src/app/util/render_text.cpp
Outdated
|
||
for (int i=0; i<info.glyphCount; ++i) { | ||
auto rc = info.getGlyphBounds(i); | ||
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height);
^
src/app/util/render_text.cpp
Outdated
|
||
for (int i=0; i<info.glyphCount; ++i) { | ||
auto rc = info.getGlyphBounds(i); | ||
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height);
^
src/app/util/render_text.cpp
Outdated
|
||
for (int i=0; i<info.glyphCount; ++i) { | ||
auto rc = info.getGlyphBounds(i); | ||
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: narrowing conversion from 'float' to 'int' [bugprone-narrowing-conversions]
m_bounds |= gfx::Rect(rc.x, 0, rc.w, height);
^
757f937
to
3456fc2
Compare
This new font selector list installed fonts with its proper name. It needs some extra work yet to select font set styles (regular, bold, italic, etc.)
If we clicked bold/italic, and then choose another font family, we were using the cached typeface inside the FontInfo instead of an update typeface with the selected styles applied (bold/italic). Now we don't cache the typeface inside FontInfo to avoid this.
…t run This fixes the required output image size to render text in different languages when the text does't support the full range of the specified chars/code points.
This is the first (not yet production-ready) version of the interactive Text tool. The text input is done with a transparent ui::Entry, and on each text modification an ExtraCel is rendered with this same ui::Entry's TextBlob to be displated in the canvas with the active zoom level. The ui::Entry is being painted along the text in the canvas (just for testing), but this is something to be fixed. Probably it will not be the case in the future and a fully customized rendering (onPaint()) process will be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
// static | ||
FontInfo FontInfo::getFromPreferences() | ||
{ | ||
Preferences& pref = Preferences::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'pref' of type 'Preferences &' can be declared 'const' [misc-const-correctness]
Preferences& pref = Preferences::instance(); | |
Preferences const& pref = Preferences::instance(); |
|
||
void FontInfo::updatePreferences() | ||
{ | ||
Preferences& pref = Preferences::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'pref' of type 'Preferences &' can be declared 'const' [misc-const-correctness]
Preferences& pref = Preferences::instance(); | |
Preferences const& pref = Preferences::instance(); |
public: | ||
FontSelector(ContextBar* contextBar) { | ||
// Load the font from the preferences | ||
Preferences& pref = Preferences::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'pref' of type 'Preferences &' can be declared 'const' [misc-const-correctness]
Preferences& pref = Preferences::instance(); | |
s const |
} | ||
|
||
~FontSelector() { | ||
Preferences& pref = Preferences::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'pref' of type 'Preferences &' can be declared 'const' [misc-const-correctness]
Preferences& pref = Preferences::instance(); | |
{ const |
@@ -2200,6 +2220,11 @@ | |||
(tool->getInk(0)->isSlice() || | |||
tool->getInk(1)->isSlice()); | |||
|
|||
// True if the current tool is text tool. | |||
const bool isText = tool && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: implicit conversion 'tools::Tool *' -> bool [readability-implicit-bool-conversion]
const bool isText = tool && | |
.( != nullptr) |
|
||
void WritingTextState::onEnterState(Editor* editor) | ||
{ | ||
StateWithWheelBehavior::onEnterState(editor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: qualified name 'StateWithWheelBehavior::onEnterState' refers to a member overridden in subclass; did you mean 'app::StandbyState'? [bugprone-parent-virtual-call]
StateWithWheelBehavior::onEnterState(editor); | |
app::StandbyState::onEnterState(editor); |
else { | ||
editor->releaseMouse(); | ||
return KeepState; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use 'else' after 'return' [readability-else-after-return]
else { | |
editor->releaseMouse(); | |
return KeepState; | |
} | |
editor->releaseMouse(); | |
return KeepState; | |
|
||
void WritingTextState::onFontChange() | ||
{ | ||
FontInfo fontInfo = App::instance()->contextBar()->fontInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'fontInfo' of type 'FontInfo' can be declared 'const' [misc-const-correctness]
FontInfo fontInfo = App::instance()->contextBar()->fontInfo(); | |
FontInfo const fontInfo = App::instance()->contextBar()->fontInfo(); |
const FontInfo& fontInfo, | ||
const std::string& text) | ||
{ | ||
text::FontRef font = get_font_from_info(fontInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'font' of type 'text::FontRef' (aka 'Ref') can be declared 'const' [misc-const-correctness]
text::FontRef font = get_font_from_info(fontInfo); | |
text::FontRef const font = get_font_from_info(fontInfo); |
const std::string& text, | ||
gfx::Color color) | ||
{ | ||
text::FontRef font = get_font_from_info(fontInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'font' of type 'text::FontRef' (aka 'Ref') can be declared 'const' [misc-const-correctness]
text::FontRef font = get_font_from_info(fontInfo); | |
text::FontRef const font = get_font_from_info(fontInfo); |
A work-in-progress to convert the "insert text" command into an interactive "text tool."