Skip to content

Fix incorrect height adjustment of the command line#5336

Merged
vadi2 merged 1 commit intoMudlet:developmentfrom
chrio:commandline-height-adjustment-fix
Jul 31, 2021
Merged

Fix incorrect height adjustment of the command line#5336
vadi2 merged 1 commit intoMudlet:developmentfrom
chrio:commandline-height-adjustment-fix

Conversation

@chrio
Copy link
Copy Markdown
Contributor

@chrio chrio commented Jul 28, 2021

Brief overview of PR changes/additions

Update the minimum height calculation in TCommandLine::adjustHeight method. Keeps the minimum size of the command line to be consistent with the icons next to the command line.

Motivation for adding to Mudlet

Fixes the behaviour where the command line height grows as soon as you start to typing. The old behaviour was making the minimum command line height twice that of the font height, which for many users is bigger than the icons next to it.

Other info (issues closed, discussion etc)

Release post highlight

Improved the command line height adjustment when typing

@chrio chrio requested a review from a team as a code owner July 28, 2021 22:15
@chrio chrio requested a review from a team July 28, 2021 22:15
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Jul 28, 2021

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@chrio chrio force-pushed the commandline-height-adjustment-fix branch from bf4c773 to 91d02cb Compare July 28, 2021 22:17
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@Xabre666
Copy link
Copy Markdown
Contributor

One observation: how does it act on a HiDPI screen?
Looks to me that you're setting the command line to min 31px, which may be tricky on such screens.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Jul 29, 2021

It's looking good on a 2K display (acer chromebook 713). Using a green command line to separate it from the main text -

setCmdLineStyleSheet("main", [[
  QPlainTextEdit {
    background: rgb(0,100,0);
    color: rgb(0,200,255);
  }
]])

It's looking correct -
Screen recording 2021-07-29 12.58.48 PM.zip

But it starts to cut off letters at the bottom if you use a ridiculous font size (which people can), like 20. Could you test?

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Jul 29, 2021

Here is my prior attempt at this, could you have a look that this solution isn't affected by problems encountered there? #2725

@chrio
Copy link
Copy Markdown
Contributor Author

chrio commented Jul 30, 2021

For really big font sizes the users have to compensate by increasing the minimum command line height.

Currently I have hardcoded a minimum possible height to 31 pixels (taken from buttonLayer in TConsole.cpp).
I'm fine with removing this limit and allow the users to have a thinner input line, but currently the commandline is created at 31px height and will shrink or grow once the user starts typing.

Should I remove the 31 pixel limit? If the users use 31+ in the settings, the behaviour is the same with or without the limit.

The important part is that the extra margin only gets added when we reach 2 lines or more.

int marginH = lines > 1 ? fontH : 0;
int _height = fontH * lines + marginH;

I wouldn't mind looking into other improvements, but I think this small change fixes this specific issue with the input line.

Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Yep it is good. We could address the large fonts separately in another PR - by suggesting a change with a link the button could press for example.

Will give others another day to review, then the PR will be open for 3 days so good to merge 👍

@vadi2 vadi2 merged commit 57e2e03 into Mudlet:development Jul 31, 2021
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Jul 31, 2021

Thanks for helping solve this, @chrio!

@chrio chrio deleted the commandline-height-adjustment-fix branch August 4, 2021 09:00
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