pop-os / mouse-configurator

Configurator for HP 935 Creator Wireless Mouse
MIT License
8 stars 2 forks source link

Bug: Some binding descriptions overlap the diagram of the mouse #7

Closed leviport closed 2 years ago

leviport commented 2 years ago

Some binding descriptions are long enough that they overlap the image of the mouse. The longest ones are display brightness up/down and move to workspace above/below. 2022-06-03_13-34

WatchMkr commented 2 years ago

@christian-salgado @maria-komarova please take a look at how we might improve this.

maria-komarova commented 2 years ago

I was actually wondering about it and I thought we should set a character limit and show full descriptions on hover.

ids1024 commented 2 years ago

I was wondering if it should show on multiple lines, but there isn't really space for that.

It's a bit awkward to address because currently each label has a hard-coded coordinate for the end of the line. I guess it would need to also specify coordinates for the other end.

WatchMkr commented 2 years ago

The longest descriptions aren't too far off from the mouse outline. What do you think about extending the beginning of the line so they'd fit? Are there other consequences to that?

maria-komarova commented 2 years ago

It's a bit awkward to address because currently each label has a hard-coded coordinate for the end of the line. I guess it would need to also specify coordinates for the other end.

Yeah, probably. Talking to @christian-salgado and thinking about it more we might not be able to rely on character limit either considering other typefaces can have a different character width. So we will need to restraint the length of the button. We can go a different route and place all the labels on one side of the mouse which would give us more length for the labels and maybe less custom coordinates although I'm not entirely sure about the latter. But then that would mean more changes overall and more tweaking of button placement.

If we limit the length we might also want to adjust the hover state. We have a hover state in the app already but I wonder if that needs to be refined in terms of styling. @ids1024 would it be possible for us to get a bit more padding around the labels in GtkButtons without loosing the placement?

I was wondering if it should show on multiple lines, but there isn't really space for that.

I was wondering about that as well but I agree, doesn't seem to be space there and might look weird in any case if we do that.

maria-komarova commented 2 years ago

The longest descriptions aren't too far off from the mouse outline. What do you think about extending the beginning of the line so they'd fit? Are there other consequences to that?

More implementation for @ids1024, there will be a need to move things around again and adjust the coordinates. Plus the alignment of widgets like the dropdown above. Primary reason why we didn't increase the length. Another consideration is it might not ensure all typefaces will behave equally. Unless changing the typeface for the system in Tweaks won't change it for this app.

maria-komarova commented 2 years ago

I don't know if we're going to have a translation for the app either. If that is not a concern, then that won't have an effect. But if we plan to have it, then labels can get longer in other languages. For example, "Move to workspace above" is only 23 characters in English but 36 in Polish (running it through the quick translator).

WatchMkr commented 2 years ago

Good point.

maria-komarova commented 2 years ago

We can increase the length of lines and adjust the coordinates to cover our defaults and English better but we'd still need to restrict the length of the buttons to account for possible edge cases.

christian-salgado commented 2 years ago

Should we just simplify the text? Workspace Up, Workspace Below, Brightness Up, Brightness Down.

maria-komarova commented 2 years ago

I think this is too cryptic. We need indicate what exactly happens with workspaces and what brightness we mean.

WatchMkr commented 2 years ago

That could work with some adjustment... "Display Brightness Up" and "Display Brightness Down" are a little shorter. "Workspace Up" and "Workspace Down" are good.

Separate but something to consider, with COSMIC DE, we'd like to enable vertical or horizontal workspaces which makes the language tricky. We may want to use "Workspace Next" and "Workspace Previous" or similar to start making it orientation agnostic,

christian-salgado commented 2 years ago

Are we able to define the define the text alignment inside the buttons? Use set_alignment for the buttons so they're right-aligned on the left side, and left-aligned on the right side. Using the white space on the sides to our advantage, we could have a high character limit.

https://docs.gtk.org/gtk3/method.Button.set_alignment.html

An example of it - overlap the diagram redesign

Here's an alternative with the shortening the svg lines and centering the buttons with the lines. overlap the diagram redesign(1)

ids1024 commented 2 years ago

Are we able to define the define the text alignment inside the buttons? Use set_alignment for the buttons so they're right-aligned on the left side, and left-aligned on the right side.

That's kind of how it already works, but reversed. Currently the labels on the left side are positioned based on their bottom left corner, while the ones on the right are positioned based on their bottom right. This way it would be aligned by the other side, and vertically centered (I think https://docs.gtk.org/gtk4/enum.ConstraintAttribute.html#center_y will do that).

For the current version, I kind of awkwardly manually adjusted the positions of labels in code until they seemed right (specifying the position of the end of each line as a percentage of width and height)... I guess I should see if that can be done in a better way if we need to change them all.

ids1024 commented 2 years ago

I guess I should see if that can be done in a better way if we need to change them all.

Okay, it looks like this can cleanly be done by pixel positions, after multiplying by pixel height/width (there were issues with how positioning worked in earlier versions, but it seems I got it to work in a sane way).

I think that alternate version should work. I'll need the the svg and the x/y positions for the end of each line, as shown in Inkscape.

maria-komarova commented 2 years ago

If we go this route, there are two smaller considerations, mostly visual:

For the first one we can at least make sure all the lines start in the same spot on each side. For window resizing, not sure what we can do. But that's probably less of a concern unless we do have translations for this app.

christian-salgado commented 2 years ago

(Updated to reflect the new alignment so there's so staircase affect.) Here's the x/y position for the alternative version as well as the SVG if we decide to go with it.

Left Click - x294 y234 Middle Click - x515 y187 Right Click - x565 y234 Left Scroll - x294 y267 Right Scroll - x565 y267 Forward - x262 y348 Super - x262 y377 Back - x262 y406

mouse-light(2)

mouse-dark(2)

ids1024 commented 2 years ago

Those coordinates don't seem to be quite right...

Screenshot from 2022-06-06 17-00-34

christian-salgado commented 2 years ago

I highlighted the point in Inkscape and got the coordinates, unless I miss understood. I can give you the Figma coordinates instead-

Left Click - x121 y64 Middle Click - x342 y17 Right Click - x392 y64 Left Scroll - x121 y97 Right Scroll - x392 y97 Forward - x89 y178 Super - x89 y207 Back - x89 y236

image

ids1024 commented 2 years ago

Not sure why you were seeing different values in Inkscape, but anyway, https://github.com/pop-os/mouse-configurator/commit/55fccae33c321288e9cc912accbcf78dbb40885e updates this:

Screenshot from 2022-06-07 08-20-17

I guess to match the mockup there should be some padding on the sides of the labels to space them from the lines (which is easy to add), but I kind of like it like this.

christian-salgado commented 2 years ago

That was odd, not sure either. But I would add an 8-pixel padding from the line.

ids1024 commented 2 years ago

Screenshot from 2022-06-07 08-34-49

ids1024 commented 2 years ago

Closing as resolved, since we seem to be happy with this design.