Skip to main content

Please review my fix for cr6486735

5 replies [Last post]
andreytitov
Offline
Joined: 2009-03-13

Text in dropdown list in Files tab was too long when file path was long

Added a library StringCutter with some methods which truncate a String using FontMetrics and available space (e.g. width of Component)

https://jtharness.dev.java.net/source/browse/jtharness?view=rev&rev=1604

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
andreytitov
Offline
Joined: 2009-03-13

Konstantin Romanovskiy requested tooltips showing full filepaths

https://jtharness.dev.java.net/source/browse/jtharness?view=rev&rev=1636

bkurotsu
Offline
Joined: 2004-12-13

Ok, not testing it right now, but code review comments:

1) The javatest.util package is a "core" package and can't reference any GUI packages. I suggest moving the new class to the javatest.tool package and renaming it slightly. StringFitter maybe? Other suggestions welcome.

2) Excellent that many methods are static. Not sure if it's great that the replacement text is static though - means you can only use it for one purpose in the VM. Perhaps change this to a singleton? javatest.tool.StringFitter.getPathFitter()?

3) Can you improve the javadoc a little? The descriptions don't quite describe what each method does. Maybe an example would be best - "this/is/a/long/path/today.txt" becomes "this/is/.../today.txt".

There are many places in the GUI which need help from this utility - for example, the fields at the bottom which display the interview and template file paths.

The critical thing for integration is the package change. Would be nice if the other things can be completed now rather than later though.

Thanks!

andreytitov
Offline
Joined: 2009-03-13

> The javatest.util package is a "core" package and can't reference any GUI packages. I suggest moving the new class to the javatest.tool package
oh, I hope I will understand package structure soon

> Perhaps change this to a singleton? javatest.tool.StringFitter.getPathFitter()?
I'm not sure I've understood you right... I have made truncation methods non-static too so correct me if it isn't what you wanted
https://jtharness.dev.java.net/source/browse/jtharness?view=rev&rev=1618

bkurotsu
Offline
Joined: 2004-12-13

The function of the code seems fine. Integrated to trunk.

I guess the getInstance() you added is ok. Perhaps I was thinking that you would have instance members that were:
private String replaceString = "...";
private String splitString = "/";

Then have a constructor that accepted either () or (String,String) to set these values. Since the methods are now non-static, splitString doesn't need to be a parameter.

The javadoc should say that "..." and "/" are the defaults. And so should the default constructor.

bkurotsu
Offline
Joined: 2004-12-13

Wow, this sounds great, I can't wait to try it! Good job Andrey.