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

Fix scrollbar wrongly displayed bug #487

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Conversation

tompng
Copy link
Member

@tompng tompng commented Dec 26, 2022

Fixes these scrollbar bugs

  • Scrollbar is not shown at bottom when I scrolled to bottom
  • Pink color is remained in scrollbar area
  • Scrollbar sometimes disappears after you scroll or when dialog size change (not shown in the gif below)
  • Scrollbar will disappear if contents is very long (not shown in the gif below)
    scroll

Change 1

         pointer = dialog.pointer - dialog.scroll_top
+      else
+        dialog.scroll_top = 0
       end

Resets scroll_top to zero if dialog.pointer == nil. It will be nil when dialog_render_info.contents changed.
Originally, reset is done at dialog.scroll_top = dialog.contents.size - height that I deleted in change2.

Change 2

-    if dialog.contents and dialog.scroll_top >= dialog.contents.size
-      dialog.scroll_top = dialog.contents.size - height
-    end

The original code unintentionally resets dialog.scroll_top to zero because dialog.contents.size - height is always zero.
This will wrongly calculates scroll_top when scroll_top >= height.
I think the purpose of the original code is to satisfy the condition 0 <= scroll_top and scroll_top + height <= dialog_render_info.contents.size when the content shrinks.
After I add dialog.scroll_top = 0 in change1, the condition seems to be always satisfied.

Change 3

+      bar_height = 1 if bar_height.zero?

If the dialog content is too long, calculated bar_height will be zero.
Since most scrollbar in GUI apps has a minimum hight, I set the minimum hight to 1.

Change 4

-      if dialog.scrollbar_pos and (dialog.scrollbar_pos != old_dialog.scrollbar_pos or dialog.column != old_dialog.column)
+      if dialog.scrollbar_pos

The condition I deleted is for optimization to skip re-rendering scrollbar.
When pointer moved or dialog contents changed, scrollbar needs to be re-rendered. This is almost always, so I deleted the optimization condition.

if dialog_render_info.scrollbar and dialog_render_info.contents.size > height
bar_max_height = height * 2
moving_distance = (dialog_render_info.contents.size - height) * 2
position_ratio = dialog.scroll_top.zero? ? 0.0 : ((dialog.scroll_top * 2).to_f / moving_distance)
bar_height = (bar_max_height * ((dialog.contents.size * 2).to_f / (dialog_render_info.contents.size * 2))).floor.to_i
bar_height = 1 if bar_height.zero?
Copy link
Member

Choose a reason for hiding this comment

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

This 1 means minimum value. It is better to avoid magic numbers and use constants.

Copy link
Member Author

Choose a reason for hiding this comment

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

I Added MINIMUM_SCROLLBAR_HEIGHT = 1

@hasumikin
Copy link
Collaborator

LGTM!

@ima1zumi ima1zumi merged commit 3c98dbc into ruby:master Jan 18, 2023
@tompng tompng deleted the fix_scrollbar branch January 18, 2023 16:10
@ima1zumi ima1zumi added the bug Something isn't working label Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants