-
-
Notifications
You must be signed in to change notification settings - Fork 373
info: indent wrapped annotation lines (fixes #3914) #3963
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
base: develop
Are you sure you want to change the base?
info: indent wrapped annotation lines (fixes #3914) #3963
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.
This is looking good, but some comments below.
It would be great to have a test for this new functionality, too, just to prevent regressing it.
const size_t valueWidth = termWidth > approxNameCol ? termWidth - approxNameCol : termWidth; | ||
|
||
for (const auto& anno : task.getAnnotations()) { | ||
// first line looks exactly like before: <indent><timestamp><space> |
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.
"before" won't mean much to a reader outside the context of this PR -- rephrase to the context of the code
// first line looks exactly like before: <indent><timestamp><space> | |
// first line looks like <indent><timestamp><space> |
static std::string wrapWithPrefixes(const std::string& text, size_t width, | ||
const std::string& prefixFirst, const std::string& prefixCont) { |
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.
There's existing wrapping support in src/libshared/src/shared.cpp, and I think that could be re-used for this purpose rather than re-implementing it.
const size_t termWidth = Context::getContext().getWidth(); | ||
const size_t approxNameCol = 20; // conservative; covers labels like "Last modified" | ||
const size_t valueWidth = termWidth > approxNameCol ? termWidth - approxNameCol : termWidth; |
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.
Hm, this seems a bit of a hack, as it's "guessing" the size of the [Table](https://github.com/GothenburgBitFactory/libshared/blob/master/src/Table.h)
column, and wrapping to that length. I suppose if the guess is incorrect you just get some weirdly line-broken text, so not a horrible bug. But maybe we could do better?
If that requires improvements to the libshared repo, that's fine -- it's only used by Taskwarrior and Timewarrior so we can modify it pretty easily.
No description provided.