pop-os / mouse-configurator

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

Bug: entering a very long string for a configuration profile title makes the entire window increase to an extremely large width #6

Closed leviport closed 2 years ago

leviport commented 2 years ago

Steps to reproduce:

  1. Edit profile name
  2. Enter some text
  3. ctrl + a to select the text, then ctrl + v a few times to make the name even longer
  4. Save the title (either enter or the checkmark button)

https://user-images.githubusercontent.com/13512097/171917269-68c534ec-789c-4a5f-9fe7-0e45958320eb.mp4

leviport commented 2 years ago

On a couple occasions now, I've also been able to crash the configurator completely with certain profile names. It allows emoji in profile names, and copy/pasting very long strings of emoji can crash the whole application.

ids1024 commented 2 years ago

Arbitrarily long strings probably can't be handled elegantly in the UI. Should we set a maximum character count for the GtkEntry? What maximum would be reasonable?

It allows emoji in profile names, and copy/pasting very long strings of emoji can crash the whole application.

Hm, I'm not sure how that could lead to a crash. I guess a bug in GTK itself?

TheCodeTherapy commented 2 years ago

I'm not experienced with GTK, or if it has some OOB method for that in a dropdown selector, but can't we treat the string for its exhibition on the GUI instead of limiting its size?

I considered having something like:

pub fn truncate(origin_str: &str, max_length: u64) -> String {
  let mut truncated = origin_str.to_string();
  if truncated.len() as u64 > max_length {
      let truncated_length = max_length - 3;
      truncated.truncate(truncated_length as usize);
      truncated += "...";
  }
  truncated
}

if that makes sense.

leviport commented 2 years ago

Hm, I'm not sure how that could lead to a crash. I guess a bug in GTK itself?

Yeah I think it was a GTK bug, and it was probably related to the massive width of the window. If I went into the config file and removed a few of the emoji, I could get the application to boot, but I saw crazy visual glitches on the right side of the window when I dragged it off the left edge of the screen.

A character limit makes the most sense to me. I don't think anyone really needs to insert the full text of War and Peace as the name of a mouse profile. As for the max, I think 20 or 30 characters should be sufficient, right? Configuration Three is the longest default, weighing in at 19 characters. I can fit 29 capital W's in the field before it hits the edge and starts to make the window bigger (but one W bigger than default is probably no big deal).

ids1024 commented 2 years ago

I guess that makes sense since GTK may not be very happy trying to allocate a window larger than available space (not that it should crash).

30 characters seems like a reasonable limit, though I don't know what users might try. Allowing longer text but ellipsizing to 30 characters in the dropdown with something like what @MarcoGomezGT suggests would be fairly easy too, but it might be strange to allow more characters only to not actually show them.

@pop-os/ux Any thoughts on this?

maria-komarova commented 2 years ago

I think both would work. Maybe character limit seems the easiest simply to avoid having to then show long lines in the dropdown. I strongly doubt people would benefit from having the ability to create very long names.

ids1024 commented 2 years ago

https://github.com/pop-os/mouse-configurator/commit/34c5f3ce4ed87988d1b32a990b89a6a88a5c3728 limits it to 30 characters. Hopefully that's sufficient.

leviport commented 2 years ago

Fix confirmed, 30 character limit keeps the window from expanding