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 close history view toolbar button #649

Merged
merged 2 commits into from
Apr 6, 2024

Conversation

codokie
Copy link
Contributor

@codokie codokie commented Apr 5, 2024

Demo:
button
See #403

@Helium314
Copy link
Owner

The keyCode seems pretty unnecessary. Why not manage it directly in ClipboardHistoryView?
Also please use a less generic name for the button.

@codokie
Copy link
Contributor Author

codokie commented Apr 5, 2024

@Helium314 How can we access the KeyboardSwitcher from inside the view?
I figured you might want to implement another view similar to the clipboard history view in the future so I tried to stay general..

@Helium314
Copy link
Owner

I figured you might want to implement another view similar to the clipboard history view in the future so I tried to stay general.

I can't remember any plan... was it something I wrote?

How can we access the KeyboardSwitcher from inside the view?

Why do you need keyboard switcher? The ABC key also doesn't need it.

@codokie
Copy link
Contributor Author

codokie commented Apr 5, 2024

Why do you need keyboard switcher? The ABC key also doesn't need it.

I debugged it and the ABC stops the clipboard history view after a chain of commands that begins at onCodeInput() with parameter codePoint = KeyCode.ALPHA = -201. So it does use a KeyCode to achieve this.
I could use the same keycode but others who will read the code may not understand the reasoning.

Also, feel free to change CLOSE_VIEW to whatever you like

@Helium314
Copy link
Owner

ABC button functionality is what is wanted in #403, so I don't think doing exactly the same as ABC key would be confusing.
Adding somthing new that duplicates existing functionality in a different way is more confusing in my opinion.

So just not using a code, and handling it in ClipboardHistoryView would be a better way as I see it. Alternatively I guess adding the alphabet code to the close button should also work, in that case add the reason for the code in a comment.

Also, feel free to change CLOSE_VIEW to whatever you like

CLOSE_VIEW is a confusing mix. Either is should be clear what the key will do (which view should be closed?), or it should be actually generic. But currently it doesn't have a generic use, and I don't know which uses you have in mind.

@codokie
Copy link
Contributor Author

codokie commented Apr 6, 2024

@Helium314 is it ok now?

@Helium314
Copy link
Owner

Thanks!

@Helium314 Helium314 merged commit 38126d2 into Helium314:main Apr 6, 2024
1 check passed
@Helium314
Copy link
Owner

One (very) minor thing not related to code changes: could link the issues you're fixing with fix or fixes before?
e.g. "fixes #403".
If you do that, the issue will automatically be closed when the PR is merged.

@Helium314
Copy link
Owner

Oh, and what I didn't notice before: the differences between the "normal" and the rounded icons are very small, I can barely see them in full screen view on a large display.
I will remove one of the icons and use the same for all themes, unless you have objections.

@codokie
Copy link
Contributor Author

codokie commented Apr 6, 2024

Thanks!

Glad to lend a hand :)

If you do that, the issue will automatically be closed when the PR is merged.

Nice, I did not know that. I'll update the other open PRs that mentioned an issue

the differences between the "normal" and the rounded icons are very small

It's the exact same icon but scaled to a 960x960 viewport. I think most other rounded icons are like that as well.

I will remove one of the icons and use the same for all themes

If it does not look too small feel free to remove it, it's fine by me of course

@Helium314
Copy link
Owner

If it does not look too small feel free to remove it, it's fine by me of course

As far as I know the viewport dimensions are only relevant for the path data. The image will always be scaled to width and height. (e.g. of you divide all numbers in pathData by 10 and do the same for the viewport, in the app there should not be any visible difference)
see also https://stackoverflow.com/questions/41353732/what-is-the-impact-of-viewportwidth-height-on-android-drawables

Helium314 added a commit that referenced this pull request Apr 6, 2024
and rename closeView to close, as the icon can be more generic than the toolbar key
@codokie codokie deleted the close_history_view branch April 14, 2024 11:32
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.

2 participants