[rfc][icedtea-web] manager for desktop integration
Jiri Vanek
jvanek at redhat.com
Thu Oct 29 15:18:33 UTC 2015
On 10/28/2015 01:23 PM, Jacob Wisor wrote:
> On 10/27/2015 at 04:11 PM Jiri Vanek wrote:
>> […]
>
> Some detailed nits:
Yes.. Mostly nits.... But generally, yes, thnax for them.
>> + Copyright (C) 2010 Red Hat
>
> Please change to 2015. ;-)
done.
One question on this topic fitting your knowledge - Do you know why the year is kept in license headers?
Unless there is some reason, I'm tempted to get rid of all of them.... (I believe there is reason
but I dont now it.).
>
>> -This program is free software; you can redistribute it and/or modify
>> -it under the terms of the GNU General Public License as published by
>> + along with this program; if not, write to the Free Software
>> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> There is actually no need to indent the license header by one space...
I had to accidentally reformat whole file. Reverted and fixed.
>
>> */
>> -
>> package net.sourceforge.jnlp.controlpanel;
>>
>> import java.awt.Component;
>> import java.awt.Dimension;
>> import java.awt.GridBagConstraints;
>> import java.awt.GridBagLayout;
>> +import java.awt.event.ActionEvent;
>> +import java.awt.event.ActionListener;
>> import java.awt.event.ItemEvent;
>> import java.awt.event.ItemListener;
>>
>> import javax.swing.Box;
>> +import javax.swing.JButton;
>> import javax.swing.JComboBox;
>> import javax.swing.JLabel;
>> +import javax.swing.JOptionPane;
>> +import javax.swing.SwingUtilities;
>> import net.sourceforge.jnlp.ShortcutDesc;
>>
>> import net.sourceforge.jnlp.config.DeploymentConfiguration;
>> +import net.sourceforge.jnlp.controlpanel.desktopintegrationeditor.LinuxIntegrationEditorFrame;
>
> If you want to have different IntegrationEditorFrames per platform then we probably should also have
> an abstract base IntegrationEditorFrame class. This is a perfect example for using abstract classes.
> Platform specific IntegrationEditorFrames should then extend and implement the abstract base class.
> The adequate class should be chosen at runtime. This approach would also make any strange dialog
> boxes about unimplemented features on any specific platform obsolete. The rule of thumb with UIs is:
> If its not implemented then simply do not show the UI. Do not bother the user with pop ups.
'm not going to implement windows part. I even have no idea how windows will (if ever) bee doing this.
So long story short, I'm not going to make abstraction on unknown thing. Especially one which never
will be used.
Anyway obeyed - stupid (yah...) popup removed, and button is disabled on Win with tooltip why.
>
>> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
>> import net.sourceforge.jnlp.runtime.Translator;
>>
>> /**
>> * This class provides the panel that allows the user to set whether they want
>> * to create a desktop shortcut for javaws.
>> - *
>> - * @author Andrew Su (asu at redhat.com, andrew.su at utoronto.ca)
>> - *
>
> Nah, just leave it and yourself.
>
>> + *
>> + *
>
> Or, dump one more line please.
dumped.
Author kept removed. Itw do not add authors to license headers, unless there is specific reason.
Andrew had wrongly set up IDE in time he was working on ITW - thats all. I tried to contact him,
(for different reason), and the contact addresses are not valid anyway.
>
>> */
>> public class DesktopShortcutPanel extends NamedBorderPanel implements ItemListener {
>>...
>> Translator.R("CPDesktopIntegrationLinuxOnly"));
>
> No wired message dialog boxes, please. Instead, as described above, stick to the abstract-concrete
> class model and do not show the IntegrationEditorFrame and button on unsupported platforms.
>
> Besides, when it comes to naming LinuxIntegrationEditorFrame, I think
> FreedesktopIntegrationEditorFrame would be a better fitting name would, since this does not only
> apply to Linux but all Freedesktop implementations.
Renmed. NAd as already written, popup removed (thanx!)
>
>> + } else {
>> + if (integrationManagment == null) {
>> + integrationManagment = new LinuxIntegrationEditorFrame();
...
>> + blinking = true;
>> + new Thread(new BlinkThreadBody()) {
>> +
>> + }.start();
>
> Why the empty block? Drop the braces before .start().
Probably some error. Removed.
>
>> + }
>> +
>> + class BlinkThreadBody implements Runnable {
>> +
>> + public BlinkThreadBody() {
>> + }
>> +
>> + @Override
>> + public void run() {
>> + final Color base = compToBlink.getBackground();
>> + for (int i = 0; i < 5; i++) {
>> + try {
>> + Thread.sleep(100);
>
> Again, no thread sleeps please. If you want to implement blinking or any other periodic events then
> use the Timer/TimerTask or ScheduledThreadPoolExecutor classes.
movet to timer. thanx.
>
>> + SwingUtilities.invokeLater(new Runnable() {
>> +
>> + @Override
>> + public void run() {
>> + if (compToBlink.getBackground().equals(base)) {
>> + compToBlink.setBackground(Color.YELLOW);
>
> Ugh, very unwise. What if the default background color is yellow? Or white? Then things get very
> quickly unreadable. Please, never use hard coded colors in the UI. You can try calculating
> acceptable contrasts from user's default colors but this may prove difficult. You can also use the
> user's default highlight colors for background and text. The later is definitely preferable.
>
oook. Made it computed. Used equation have quite good results on aprox 20 backgroudns I tried with a
wide spectre as possible.
> Can you use the <blink> element?
no (not working...)
>
>> + } else {
>> + compToBlink.setBackground(base);
>> + }
>> + }
>> + });
>> + } catch (InterruptedException ex) {
>> +
>
> Err, we definitely need code here to stop blinking and reset to default colors if anything goes
> wrong to restore readability! Or, of not here then definitely in a finally block, which would
> probably be best.
It was done even in original code, and is done in same way now. Once blinking is done, new call
setting the original color "back for sure" is added to eventQueue
>
>> + }
>> + }
>> + SwingUtilities.invokeLater(new Runnable() {
>> +
>> + @Override
>> + public void run() {
>> + compToBlink.setBackground(base);
>> + blinking = false;
>> + }
>> + });
>
> I do not think this invokeLater() is really necessary.
oh it is very much necessary...
I really wont tha last thing in avtQueue left by Blinker to be reset to base colour
>
>> + }
>> + }
>> +}
>> diff -r 56bfa957a6b8 netx/net/sourceforge/jnlp/controlpanel/desktopintegrationeditor/JListUtils.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> + private static Map<File, Icon> iconCache = new HashMap<>();
>> + private static Map<File, String> texFilesCache = new HashMap<>();
>
> Spelling mistake?
fixed
>
>> + private static Map<File, Long> stamps = new HashMap<>();
>> +
...
>> +
>> +
>
> Wow, so many empty lines!
fixed
>
>> + }
>> +
>> + public static class FileBasedList implements ListModel {
>
> FileBasedList is a terrible name. This sounds very generic although obviously this class' purpose is
> very specific.
renamed
>
>> +
>> + private final File directory;
>> + private File[] list;
>> + private final String mask;
>> +
>> + public FileBasedList(File file) {
>> + this(file, ".*");
>
> Please document why you call with mask = ".*" here.
done
>
>> + }
>> +
>> + protected File getFile() {
>> + return directory;
>> + }
>> +
>> + @Override
>> + public String toString() {
>> + return getFile().getAbsolutePath();
>> + }
>> +
>> +
>> +
>
> Even more empty lines.
even more lines removed
>
>> + public FileBasedList(File file, final String mask) {
>
> Please Javadoc this constructor, since I am having difficulties to understand why it is needed and
> why it is called in FileBasedList(File file) with mask = ".*".
>
>> + directory = file;
>> + this.mask = mask;
>> + }
>> +
>> + private File[] populateList() {
>> + list = getFile().listFiles(new FilenameFilter() {
>> +
>> + @Override
>> + public boolean accept(File dir, String name) {
>> + return (name.matches(mask));
>
> Why do we need to hold a reference to an instance of the mask for the entire lifetime of FileBasedList?
Because it have no size at all, and popoulation of list is done on demand. Anyway - alter in review
you complained about not-compiled regexes. So here I changed from String to pattern and keep
compiled regex.
>
>> + }
>> + });
>> + return list;
>> + }
>> +
>> + @Override
>> + public int getSize() {
>> + if (list == null) {
>> + populateList();
>> + }
>> + return list.length;
>> + }
>> +
>> + @Override
>> + public Object getElementAt(int index) {
>> + if (list == null) {
>> + populateList();
>> + }
>> + return list[index];
>> + }
>> +
>> + @Override
>> + public void addListDataListener(ListDataListener l) {
>> +
>> + }
>> +
>> + @Override
>> + public void removeListDataListener(ListDataListener l) {
>> +
>> + }
>> +
>
> More empty lines.
>
No more empty lines
>> + }
>> +
>> + public static class CustomFileList extends JList<File> {
>> +
>> + public CustomFileList() {
>ue, index,
>> isSelected, cellHasFocus);
>> + File f = (File) value;
>> + String s = processTexFilesCache(f);
>
> Spelling mistake or TexMex? :-D
Fixed.
>
>> + if (!isSelected) {
>> + if (isJavaws(s)) {
>> + l.setBackground(new Color(0, 200, 0));
>
> Do not use hard coded colors.
>
>> +
>> + } else if (isBrowser(s)) {
>> + l.setBackground(new Color(100, 150, 0));
>
> Do not use hard coded colors.
>
>> + } else {
>> + l.setBackground(new Color(255, 200, 200));
>
> Do not use hard coded colors.
>
>> + }
>> + } else {
>> + if (isJavaws(s)) {
>> + l.setForeground(new Color(0, 200, 0));
>
> Do not use hard coded colors.
>
>> +
>> + } else if (isBrowser(s)) {
>> + l.setForeground(new Color(100, 150, 0));
>
> Do not use hard coded colors.
>
>> + } else {
>> + l.setForeground(new Color(255, 200, 200));
>
> Do not use hard coded colors.
And what colours you would like ot have here?
Is there some "system green" :system red" and "system dark green" ? If so I will probably use the.
Otherwise I will stay with this hardcoded green for green and red for red ....
>
>> + }
>> + }
>> + return l;
>> +
>
> Ugh, empty lines again.
Hopefully not anymore.
>
>> + }
>> +
>> + private boolean isJavaws(String s) {
>> + return haveString(s, "javaws");
>> +
>> + }
>> +
>> + private boolean isBrowser(String s) {
>> + String[] browsers = XDesktopEntry.BROWSERS;
>> + for (String browser : browsers) {
>> + if (haveString(s, browser)) {
>> + return true;
>> + }
>> + }
>> + return false;
>> + }
>> +
>> + private boolean haveString(String s, String i) {
>> + return s.matches("(?sm).*^.*Exec.*=.*" + i + ".*$.*");
>
> This regex should probably be stored pre-compiled in a static final variable.
I miss some skill there - You can compile regex with param?
>
>> + }
>> + }
>> +
>> + private static class IconisedCellRenderer extends DefaultListCellRenderer {
>> +
>> + @Override
>> + public Component getListCellRendererComponent(
>> + JList list, Object value, int index,
>> + boolean isSelected, boolean cellHasFocus) {
>> +
>> + File f = (File) value;
>> + JLabel label = (JLabel) super.getListCellRendererComponent(
>> + list, value, index, isSelected, cellHasFocus);
>> + label.setIcon(processIconCache(f));
>> + label.setText(f.getName());
>> + label.setHorizontalTextPosition(JLabel.RIGHT);
>
> Probably works for ltr scripts only. Please work around this or test with rtl scripts.
Who cares on which side of filename is icon it represents. I dont. Kept. (anyway, tried and both
looks ok)
>
>> + return label;
>> + }
>> +
>> + }
>> +
>> + private static Icon processIconCache(File f) {
>
> Although private, it needs Javadoc. What does it do?
done
>
>> + Icon i = iconCache.get(f);
>> + if (i == null) {
>> + i = updateIconCache(f, i);
>> + } else {
>> + if (f.lastModified() != stamps.get(f)) {
>
> What about read I/O errors here?
Nothing? All ioexception occurences are catched, and proecessed by returnin null (icon) or
exception itself (text) and both those reuslts are handled correctlya nd working.
>
>> + i = updateIconCache(f, i);
>> + }
>> + }
>> + return i;
>> + }
>> +
>> + private static Icon updateIconCache(File f, Icon i) {
>
> Although private, it needs Javadoc. What does it do?
done.
>
>> + i = createImageIcon(f, f.getAbsolutePath());
>> + if (i != null) {
>> + iconCache.put(f, i);
>> + stamps.put(f, f.lastModified());
>
> What about read I/O errors here?
same
>
>> + }
>> + return i;
>> + }
>> +
>> + private static String processTexFilesCache(File f) {
>
> Again, what does it do?
> Spelling mistake?
fixed and commentewd.
>
>> + String s = texFilesCache.get(f);
>> + if (s == null) {
>> + s = updateTextCache(f, s);
>> + } else {
>> + if (f.lastModified() != stamps.get(f)) {
>
> What about read I/O errors here?
>
sa,e
>> + s = updateTextCache(f, s);
>> + }
>> + }
>> + return s;
>> + }
>> +
>> + private static String updateTextCache(File f, String s) {
>> + s = LinuxIntegrationEditorFrame.fileToString(f, false);
>> + if (s != null) {
>> + texFilesCache.put(f, s);
>> + stamps.put(f, f.lastModified());
>
> What about read I/O errors here?
same
>
>> + }
>> + return s;
>> + }
>> +
....
>> +
>> + //gui
>> + private final javax.swing.JLabel title = new JLabel();
>> + private final javax.swing.JCheckBox selectRelatives = new JCheckBox();
>
> :-D Your variable naming gets funny sometimes. I was not aware that UI components could have
> relatives. Well sure, there are parent and child components but I do not think what you had in mind.
o lot of renaming. Simple applying and trying the patch would give you an simple answer. Anyway
renamed (togehter with others)
>
>> + private final javax.swing.JButton removeSelectedButton = new JButton();
>> + private final javax.swing.JButton cleanAll = new JButton();
...;
>> + selectRelatives.setText(R("DIMselectRelatives"));
>
> Yey, property keys get funny names too! :-D
renamed in same manner.
>
>> + reloadsListButton.setText(R("DIMreloadLists"));
...
>> + });
>> + }
>> +
>> + private boolean selecting = false;
>> + private final int expectedWidth = 800;
>> + private final int expectedHeight = 600;
>
> Why fixed sizes? Why 800 and 600? Why not just compact UI components and let the layout engine decide?
Ok. this one is tricky. And trust me, I wonted to have it like it. There are three reasons:
- first and simple - jtextpane is trying hard to minimize no matter what you do. It can be
workarounded by filling it by some eg <br><br><br><br><br><br><br>
- second when columns are empty or have short names (unlikly) then all three in left are tiny and
fourth in righ is huge. Workarounding it without fixed width is nasty cod.
- third, and most important. The names of the desktop shortcuts are very often very long.IN that
case the pack is msotly going to weight ower whole screen with very probable very wrong balance
between widths.
Long story short, the dialog looks best like this. If you will insists on pack(), then logic making
the jsplits split in balance wil be necessary anyway. Jsut the size of the dialogue will be...
really strange.
So kept as it was.
>
>> + public LinuxIntegrationEditorFrame() {
>> + setDefaultCloseOperation(javax.swing.WindowConstants.DISPOSE_ON_CLOSE);
>> + this.setSize(expectedWidth, expectedHeight);
>> + populateLists();
>> + setTexts();
>> + setListeners();
>> + setLayout();
>> + selectRelatives.setSelected(true);
>> +
>> + ListSelectionListener generatePreviewListener = new GeneratePreviewListener();
>> +
>> + iconsList.addListSelectionListener(generatePreviewListener);
>> + desktopList.addListSelectionListener(generatePreviewListener);
>> + menuList.addListSelectionListener(generatePreviewListener);
>> + generatedList.addListSelectionListener(generatePreviewListener);
>> + blinker = new Blinker(selectRelatives);
>> +
>> + }
>> +
>> + private void populateLists() {
>> + menuList.setModel(new
>> JListUtils.InfrastructureFileDescriptorBasedList(PathsAndFiles.MENUS_DIR));
>> + desktopList.setModel(new JListUtils.FileBasedList(new
>> File(XDesktopEntry.findFreedesktopOrgDesktopPathCatch()), "(?i)^.*\\.desktop$") {
>
> Again, the regex should be pre-compiled here too. And, I think "(?i)\\.desktop$" should suffice if a
> substring match is applied.
yy. compiled and reused in listmodels extensions.
>
>> + @Override
...
>> +
>
> Empty line.
Not sure here. But Ihope not.
>
>> + }
>> +
>> + private void selectAll() {
>> + selecting = true;
>> + try {
>> + selectAll(menuList);
>> + selectAll(desktopList);
>> + selectAll(iconsList);
>> + selectAll(generatedList);
>> + } finally {
>> + selecting = false;
>> + }
>> + previewPane.generatePreview();
>> + }
>> +
>> + public List<File> allItemsAsFiles(JList l) {
>> + return allItemsAsFiles(l.getModel());
>> + }
>> +
>> + public List<File> allItemsAsFiles(ListModel l) {
>> + List<File> r = new ArrayList<>(l.getSize());
>> + for (int i = 0; i < l.getSize(); i++) {
>> + r.add((File) l.getElementAt(i));
>> +
>> + }
>> + return r;
>> + }
>> +
>> + private List<File> objectListToFileList(List l) {
>> + List<File> r = new ArrayList(l.size());
>> + for (Object l1 : l) {
>> + r.add((File) l1);
>> + }
>> + return r;
>
> Very very combersome. :-( I do not think this is what ListModel and ListCellRenderer was intended
> for or how it was intended to be used. The model should hold data and the renderer should translate
> the data to visible UI. So there is no need for double list handling. Besides, you may want to take
> a look at ArrayList.addAll() or new ArrayList<File>(List.toArray(new File[0])). These are definitely
> much faster.
I disagree. This is exactly what I wont to do. Create copy of the list as correctly typed editable
collection. Non of your suggested methods do so.
>
>> + }
>> +
>> + private void removeSeleccted(List<File>... a) {
>
> Spelling mistake?
obviously :(
>
>> + for (List<File> list : a) {
>> + for (File file : list) {
>> + file.delete();
>> +
>> + }
>> + }
>> + }
>> +
>> + private int getTotal(List<File>... a) {
>> + int i = 0;
>> + for (List<File> list : a) {
>> + for (File file : list) {
>> + i++;
>
> Ugh, why not use List.size() (in combination with List.here?
> And, although improbable but theoretically possible i may overflow. ;-)
Best catch ever :D
The method was originally using something else. And I overlooked this really cool thing :D Yes may
owerflow. but hopefully will not in our universe
>
>> + }
>> + }
>> + return i;
>> + }
>> +
>> + private void findOrphans(JList possibleOrphans, List<File>... whereItCanBe) {
>> + selecting = true;
>> + if (selectRelatives.isSelected()) {
...
>> + }
>> + return s;
>> +
>> + }
>> +
>> +}
>> diff -r 56bfa957a6b8 netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Tue Oct 27 14:13:23 2015 +0100
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Tue Oct 27 15:59:17 2015 +0100
>> @@ -568,6 +568,8 @@
>> CPSecurityDescription=Use this to configure security settings.
>> CPDebuggingDescription=Enable options here to help with debugging
>> CPDesktopIntegrationDescription=Set whether or not to allow creation of desktop shortcut.
>> +CPDesktopIntegrationShowIntegrations=Show desktop and menu integrations window
>> +CPDesktopIntegrationLinuxOnly=Desktop integration manager avaiable only for Linux. Sorry
>> CPJVMPluginArguments=Set JVM arguments for plugin.
>> CPJVMitwExec=Set JVM for IcedTea-Web \u2014 working best with OpenJDK
>> CPJVMitwExecValidation=Validate JVM for IcedTea-Web
>> @@ -1000,6 +1002,36 @@
>> CVCPColPath=Path
>> CVCPColName=Name
>>
>> +# Control Panel - desktop integration manager
>> +DIMtitle=System integration shortcuts IcedTea-Web manager
>
> Too long. How about "IcedTea-Web Shortcut Manager". Please note that all words in English titles
> always start with a capital letter.
followed.
>
>> +DIMremoveSelected=Remove selected
>
> Shorten to "Remove". By design UIs are supposed to operate on selected stuff.
Kept. I really dont like magic "remove" and wondering wht it will do.
>
>> +DIMselectRelatives=Select relatives
>
> I do not think you mean realtives here. I assume you mean related items.
Wel i really wonted to use relatives in it. But related sounds same to me. And If you insists...
>
>> +DIMreloadLists=Reload lists
>
> This could be probably shortened to just "Reload".
Again. kept. no need to make it short nd less describing.
>
>> +DIMselectAll=Select all
>> +DIMclearSelection=Clear selction
>
> Unselect or Deselect.
I dont like sound of it, but as you wish...
>
>> +DIMdescription=In this window you can manage shorctuts and theirs resources (cached images and
>> generated stuff) icedtea-web created for desktop integration
>
> Should be even simpler:
> "Manage the shorctuts and resources (cached images, etc) IcedTea-Web created for desktop integration"
>
well why not...
>> +DIMguessedDesktop=Desktop folder as well guessed as possible.
>
> ??? I have no clue what this is supposed to mean, and so most users will not either. However, I am
> assuming this is some text on a label describing some other UI component actually displaying the
> desktop folder as detected by IcedTea-Web. Labels should not contain lengthy or verbose
> explanations. Keep it short and simple: "Desktop Folder:". Spare explanations for tool tips.
>
[1]
>> +DIMselectionPreview=Selection preview
>
> If this is a title or label then shorten it to "Preview" or "Preview:", repectively.
>
>> +DIMaskBeforeDelete={0} Files will be delted. Are you sure?
>
> "Are you sure you want to delete {0,number,integer} files?" Note that the number has to be
> localized. Besides, you really have to check your spelling.
>
done
>> +DIMgeneratedJnlps=Generated jnlps
>
> Acronyms of names are always all caps and no dots in English: "JNLPs".
ok!
>
>> +DIMgeneratedJnlpsTooltip=All files in this list should be generated by icedtea-web!
>
> Tool tips do not describe what /should/ happen with exclamation marks but explain in simple present
> tense what a certain control does or what function it initiates. <- Like this sence. ;-) Please also
> make sure that whenever you refer to IcedTea-Web as a product name, you stick to the name's
> "marketing" style of writing. Because it is a name, it would be even better to insert it via a
> MessageFormat argument from a String constant.
>
[1]
> I think it would be nice if the tool tip key names would follow the camel case style of eg JToolTip,
> hence you would end up with a key name like DIMgeneratedJNLPsToolTip.
[1]
>
>> +DIMicons=Icons
>> +DIMiconsTooltip=All files in this list should be icons cached by icedtea-web!
>
> See DIMgeneratedJnlpsTooltip.
>
>> +DIMorphans=Orphans
[2]
>
> I am not sure ordinary users will understand what this is supposed to mean.
Tooltip will tell them.
>
>> +DIMmenuItems=Menu items
>
> "Menu Items"
> If it is a label to another interactive UI component then please do not forget the trailing colon.
ok. It islabel, but should nothave colon.
>
>> +DIMmenuItemsTooltip=All shortcuts in this list should be generated by icedtea-web!
>
> See DIMgeneratedJnlpsTooltip.
>
>> +DIMdesktopItems=Desktop items
>
> "Desktop Items"
> If it is a label to another interactive UI component then please do not forget the trailing colon.
>
>> +DIMdesktopItemsTooltipL1=Not all your shortcuts on your were generated by icedtea-web!
>
> See DIMgeneratedJnlpsTooltip.
> "IcedTea-Web could not create all shortcuts."
>
>> +DIMdesktopItemsTooltipL2=For your convenience:
>
> I am not sure why this tool tip message ends with a colon and what the purpose shall it serve... I
> would guess that you should rather drop it entirely.
[1]
>
>> +DIMdesktopItemsTooltipL3=red items are probably not from icedtea-web
>
> See DIMgeneratedJnlpsTooltip.
> Tooltips should also contain complete sentences. In effect, tool tips are supposed to work like
> micro online documentation or help.
1] : Yes, those tooltips are exactly like this. Please run the patch and judge them in play
>
>> +DIMdesktopItemsTooltipL4=dark green items are browser shorctus, so they were probably been
>> generated by icedtea-web
>
> See DIMgeneratedJnlpsTooltip and DIMdesktopItemsTooltipL3.
> Besides, I think you meant "...have been generated...". The tense you used here does not exist.
> Again, check your spelling please.
fixed
>
>> +DIMdesktopItemsTooltipL5=green items are javaws shortcuts, so they are wery likely icedtea-web's
>
> See DIMgeneratedJnlpsTooltip and DIMdesktopItemsTooltipL3.
[1]
>
>> +DIMdesktopItemsTooltipL6=In all cases, be careful what you delete, and verify connections with
>> `select relatvves` mode
>
> See DIMdesktopItemsTooltipL3.
> I do not think this tool tip message is helpful at all, even more confusing. This message makes me
again. read [1]
> assume that there is a design problem with your UI or your approach to create shortcuts based on
> images in the cache. Remember, the user should never be able to break anything unless it is a very
> complex (and costly to reverse) operation or a irreversible operation by definition. Please also
> refrain from using ` U+0060 GRAVE ACCENT as quotation marks. See
> http://www.cl.cam.ac.uk/~mgk25/ucs/quotes.html for some decent explanation.
>
>> +DIMgeneratedButton=generated
>
> Buttons always (with very few exceptions) employ commands. I am not sure what the function of this
> button is but the command form of "generated" is "Generate". But, if that button is for generating
> or creating shortcuts from selected image files then you probably want to label the button "Create"
> or "Create Shortcut".
[2]
>
>> +DIMgeneratedButtonTooltip=Will select related generated stuff.
>
> See DIMgeneratedJnlpsTooltip and DIMdesktopItemsTooltipL3.
> Please refrain from using the word stuff in descriptions and explanations. So, turn it into
> something like "Selects related items."
[1]
>
>> +DIMiconsButton=icons
>
> Buttons employ commands. "Icons" is not a verb so you cannot put it on a button. And, do not think
> there is any reason to make an exception here. So, think about what this button does and then label
> it accordingly.
>
[2]
>> +DIMiconsButtonTooltip=Will select related cached icons.
>
> See DIMgeneratedButtonTooltip.
2]: those texts are as short as possible on purpose. The button/butons are below each column and so
they can make the column to wide or to swap themselves to that stupid ...restofText... or similar.
Theirs purpose is explained in tooltip.
>
>> +
...
>> actualResult);
>> }
>
> Phew, I can't type anymore. Maybe I'll find some additinal time to do some more review later.
>
> Regards,
>
> Jacob
Thank you for review!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: desktopIntegrationMAnager4-noIco_stringBUilders_nits.patch
Type: text/x-patch
Size: 66622 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20151029/07b58890/desktopIntegrationMAnager4-noIco_stringBUilders_nits-0001.patch>
More information about the distro-pkg-dev
mailing list