Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [udig-devel] uDig - Code Review on DocumentPropertyPage
Additional comment on functionality:
  • On click on “Restore Defaults”
    •
  • Should “Enable Hotlink” also be reset?
  • Try the ff steps:
    •
  1. Ensure that some attributes are set in the table view
  2. Click Restore Defaults
  3. Click/Unclick Enable Hotlink, the previous table entries will be set back
    4.
 
_____________________________________________
From: Nazareno Chan
Sent: Monday, 30 July 2012 3:52 PM
To: Jody Garnett; Jody Garnett
Cc: 'User-friendly Desktop Internet GIS'
Subject: uDig - Code Review on DocumentPropertyPage
 
 
Hey Jody,
 
Just some comments below on the DocumentPropertyPage.
 
UI – Document Page:
  • Suggest to clarify/standardize terms and update labels (if necessary) to avoid confusion. Eg. Documents vs. Hotlinks
  • Suggest to remove “Document” beside the “Enable Hotlink” checkbox
  • Suggest to rename “Enable Hotlink” to “Enable Hotlinks” or “Enable Hotlinking”
  • Suggest to remove extra column (to the right of Definition) on the table view
  • Definition column is never filled up and always displays “--”
  • Suggest to use “Display Name/Code” for Type column
  • Suggest to indent attributes table section to emphasize that this property is dependent on the “Enable Hotlink” checkbox
  • Suggest to highlight entire row when clicking on any column
  • Suggest to allow clicking on any column to select entire row
  • Suggest to remove clickability of headers if sorting is not supported (if possible)
    •
 
UI – Add/Edit Popup:
  • Add a title to the dialog window
  • Add “:” to the end of “Attribute” label for consistency
  • Suggest to rename “Document” to “Type”
  • Suggest to use “Display Name/Code” for Type field
  • “Action” does not have an input field and doesn’t seem to be used
    •
 
Code – General:
  • Suggest to clean up warnings like unused imports, non-externalized strings, etc.
  • Suggest to standardize spacing between variables, methods, etc. to increase readability and ease maintenance
  •
 
Code – DocumentPropertyPage:
  • Externalize strings used in the UI
  • Suggest to remove unused code. Eg. doComputeSize()
  • Suggest to rename variables to more meaningful and descriptive names making it easier to maintain. Eg. HotlinkDescriptor created = prompt.getDescriptor();
  • Suggest to set relative column widths to enable auto-adjustment when window size is adjusted
  • Refactor duplicate code used in performDefaults() and createContents(Composite parent) to ease code maintenance
    •
 
Code – DocumentPropertyPage:
  • Suggest to refactor and extract the hotlink descriptor delimiter to a variable to ease maintenance
    •
 
Code – HotlinkDescriptor:
  • Suggest to refactor and extract the hotlink descriptor definition value delimiter to a variable to ease maintenance
  • Check if it is safe to assume that type = WEB if only the attribute name is specified
    •
 
Functionality:
  • On Add, check if attribute to be added already exists in list
  • On Edit, check if attribute to be assigned already exists in list
  • Suggest to add a “Label” or “Name” value to allow user to input a label for the hotlink
  • Enable Hotlink checkbox seems to extend to the whole length of the window as it allows clicking outside of the button or text’s area
  • Clarification: Should unchecking “Enable Hotlink” and applying remove all the attribute settings or just simply disable hotlinking?
  • Clarification: Is the checkbox necessary? Or can we just check if “ hotlink attributes” count > 0?
    •
 
Thanks,
Naz Chan
 

  ________________________________  
The contents of this email are confidential and may be subject to legal or professional privilege and copyright. No representation is made that this email is free of viruses or other defects. If you have received this communication in error, you may not copy or distribute any part of it or otherwise disclose its contents to anyone. Please advise the sender of your incorrect receipt of this correspondence.

Back to the top