Skip to content
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

add FKEY-HEADER #232

Merged
merged 3 commits into from
Jan 30, 2021
Merged

add FKEY-HEADER #232

merged 3 commits into from
Jan 30, 2021

Conversation

zcsahok
Copy link
Member

@zcsahok zcsahok commented Jan 27, 2021

Sequel to #229 by @ha5se

  • removed unused variables k_tune from main and the externs from clear_display.

test/data.c Show resolved Hide resolved
sprintf(fkey_header, "%s%s%s",
spaces(left_padding), tmp, spaces(right_padding));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had done it in show_header_line, but that is another way. Fine.

Copy link
Member Author

@zcsahok zcsahok Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way it's done once at start. All else stays as-is.
When we'll have variable width, then we can re-think this and probably other stuff as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Imho, centering would possibly be better off in parse_logcfg, needed only once per config-file re-read
  • just because the next idea is almost completely implemented by now, needing only to conditionally zero out left- or right padding, would it be too much Left|Centered|Right aligning instead of centering, FKEY_HEADER=[L:|C:|R:]text, defaulting to "C:" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Centering will be just a nice to have once we implement leading/trailing whitespace for config values. This is on my list. With whitespace the user will have better control on the appearance of the text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, fully agree on whitespaces with you.

@ha5se
Copy link
Contributor

ha5se commented Jan 28, 2021

removed unused variables k_tune from main

good hit again, it is a leftover by mistake when I split my two new keywords into two branches...

@ha5se
Copy link
Contributor

ha5se commented Jan 29, 2021 via email

@zcsahok
Copy link
Member Author

zcsahok commented Jan 29, 2021

Added code to center after :CFG.

For the :XXX related issues incl. config reloading see #233.

Personally, I would not apply my last commit.

@dl1jbe
Copy link
Member

dl1jbe commented Jan 30, 2021

I see no need to rush to a quick solution.

I can understand your motivation to just keep the hands off the :CFG problem for now.
Let me just explain the reason for me disagreeing:

  • FKEY_HEADER is a feature users will likely use more than once. When it behaves weird, they will complain and send bug report to handle - more work for us. And they will not wait till we iron any problems out and prepare a new release. In the days of github they often use the development branch and build from there.
  • Other features are still wrong -right- but they seem to be seldom used (or the users are common with the problem).

So not introducing 'weird behavior' means less work for us fixing problems/explaning why it is how made it and more time for going forward.

Thus said I see some solutions for the problem:

  1. Fix :CFG behavior. I fear that is not doable in short time, so that seems not really to be a solution.
  2. Drop centering the line completely. Just print it left (or right). As soon as your planned 'Keywords with spaces' feature is available the user has enough control over the appearance of his text. Until then the behavior is at least consistent.
  3. Fall back to center it during show_header_line

What do you think?

@zcsahok
Copy link
Member Author

zcsahok commented Jan 30, 2021

I don't quite get what is still outstanding as the last commit fixes centering after :CFG. At least for me it works.

@dl1jbe
Copy link
Member

dl1jbe commented Jan 30, 2021

There was nothing wrong with your commit. I just wanted to say, that there are other ways to avoid the reformating after :CFG as you are not happy with it.

@dl1jbe dl1jbe merged commit 7194038 into Tlf:master Jan 30, 2021
@zcsahok zcsahok deleted the fkey_header branch January 30, 2021 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants