Ticket #9043 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

ReflGUI to use MS excel-style

Reported by: Owen Arnold Owned by: Keith Brown
Priority: critical Milestone: Release 3.2
Component: Reflectometry Keywords:
Cc: Blocked By: #9041
Blocking: #9039 Tester: Martyn Gigg

Description (last modified by Keith Brown) (diff)

The Users seem to want a more MS-excel styled table with dragging-style autofill, the abilty to hide/unhide columns, shortcuts to bulk-copy/cut/paste/clear cells, and want an annoyance with resizing columns fixed

Attachments

Screen Shot 2014-03-10 at 16.37.22.png (26.7 KB) - added by Owen Arnold 7 years ago.

Change History

comment:1 Changed 7 years ago by Nick Draper

  • Status changed from new to assigned

comment:2 Changed 7 years ago by Keith Brown

  • Status changed from assigned to inprogress
  • Description modified (diff)
  • Summary changed from Additional ReflGUI requirement to ReflGUI to use MS excel-style

comment:3 Changed 7 years ago by Keith Brown

Column resizing is fixed, it was simply a flag in the designer had been activated accidentally. Delete functionality is on it's way as Del has been wired up to a handler. Show/hide functionality is possible but it will require some work.

It seems that a couple of parts of this ticket may be impossible or a massive pain to implement:

Clipboard functionality is limited by the multi platform requirement. I've looked at various ways of accessing the clipboard and they're either single platform, require a new module, or not fully installed within mantid. However Owen has discovered the i can grab a static qapplication instance and get a qclipboard from that.

Draggable autofill is not being implemented as it would essentially involve inheriting from QTablewidget and overriding the paint method.

comment:4 Changed 7 years ago by Keith Brown

Refs #9043 Interface UI file changed

The column resize bug has been fixed fixed (horizontalHeaderCascadingSectionResizes was true instead of false)

The new Edit menu has been created with Copy, Cut, Paste and Clear with their universally recognised shortcuts Ctrl-C, Ctrl-X, Ctrl-V, and Del

Changeset: 42698f662c9e73cf749451fac8938310a76bd577

comment:5 Changed 7 years ago by Keith Brown

Refs #9043 Beginning connecting the Edit Menu

A clipboard object is now retrieved and stored when setting up the UI

Copy, Cut and Paste are wired up to basic handlers that simply print out at present in order to test their shortcuts.

Clear is implemented as expected, clearing text from highlighted cells when Edit->Clear or Del is pressed.

Changeset: 320c1b784d96de40168040dca76a04f2a52fa14c

comment:6 Changed 7 years ago by Keith Brown

Refs #9043 Copy, Cut, Paste and clear all work

All of the edit menu now works as expected.

Copy/Paste functionality is compatible with MS Excel, and works in a similair way to Excels pasting too in regards to behavior when the user supplies a single cell target or a multi-cell target.

Selection mode has been changed to Contiguous to prevent problems occuring with multiple selections.

Changeset: 3e24b2534a527e11360e974752b61f9b9895b470

comment:7 Changed 7 years ago by Keith Brown

All that remains now is show/hide column functionality

comment:8 Changed 7 years ago by Keith Brown

Refs #9043 Column hiding dialog

The new View menu contains 'Choose Columns...' which allows the user to select which collumns are hidden.

The dialog now uses Qsettings, so in future tickets, persistent options can be stored in there.

Changeset: 7f591575b76f1c416ebf8f04f9f572e224b1020f

comment:9 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:10 Changed 7 years ago by Keith Brown

  • Blocked By 9041 added

(In #9041) As this ticket involved heavy changes to the UI file i would expect conflicts from other tickets if they were to attempt a merge, so until this has been tested and merged any other Reflectometry tickets can't be checkbuilt.

comment:10 Changed 7 years ago by Keith Brown

  • Status changed from verify to reopened
  • Resolution fixed deleted
  • Blocking 9039 added
  • Blocked By 9041 removed

As this ticket involved heavy changes to the UI file i would expect conflicts from other tickets if they were to attempt a merge, so until this has been tested and merged any other Reflectometry tickets can't be checkbuilt.

comment:11 Changed 7 years ago by Keith Brown

  • Blocked By 9041 added

comment:13 Changed 7 years ago by Keith Brown

  • Status changed from reopened to inprogress

Refs #9043 Updated window and collun selection window

Had to commit before merging master again as i'd run my conversion scripts. all that's changed is the time generated

Changeset: 5b75eb9152b65b937cf22f38bbbe95bb97d520f9

comment:14 Changed 7 years ago by Keith Brown

Refs #9043 Merging master into branch

Merge remote-tracking branch 'origin' into feature/9043_Refl_gui_Excel_style

Conflicts:

Code/Mantid/scripts/Interface/ui/reflectometer/refl_gui.py Code/Mantid/scripts/Interface/ui/reflectometer/refl_window.py

Changeset: 50b67e60ed3de40fd85bbc18c017664c586bbb97

comment:15 Changed 7 years ago by Keith Brown

Refs #9043 Sorting out merge conflict with develop

Merge branch 'feature/9043_Refl_gui_Excel_style' into develop

Conflicts:

Code/Mantid/scripts/Interface/ui/reflectometer/refl_gui.py

Changeset: 1a2027d6a3b7c5a7a960de92ed90df0f349faf23

comment:16 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

To Tester

Test the following in the ISIS reflectometry GUI:

Choose columns works as expected in view->choose columns

Resizing columns doesn't change neighbouring columns

Copy, cut and paste work as expected (like MS Excel. especially:

  • When pasting into an area that's bigger or smaller than the source area (which should paste as much as the selected area will allow)
  • Pasting into a single cell (which should paste the entire source)
  • Pasting into a text editor produces tab-delimited data
  • Pasting into a spreadsheet program behaves like the cells came form there originally

Note that dragging auto-fill has NOT been implemented as it was deemed too much work for the end result.

comment:17 Changed 7 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:18 Changed 7 years ago by Owen Arnold

  • Status changed from verifying to reopened
  • Resolution fixed deleted

Seems to work, but I'd like to see the following changes made:

  • Dialog for selecting columns doesn't have sufficient width on the mac (see attachment)
  • Put an additional button that launches the column configuration dialog onto the GUI. That would provide a nice shortcut for accessing this useful feature.
  • Right-click copy&paste only seems to show up when a single cell is highlighted. See if there's an easy solution to this.
  • Highlighting two rows in excel and attempting to paste into the reflgui gui (single cell) only results in one row being populated.

Changed 7 years ago by Owen Arnold

comment:19 Changed 7 years ago by Keith Brown

Column Dialog fix should be easy enough, I think I just need to get it to size to the contents. Extra button is no sweat too, I'll also look into a keyboard shortcut

My theory on the pasting from excel issue may be because what's being copied is longer than the gui, should be easy enough to fix.

Context menu copy/paste may be difficult in the same way i didn't do context menu show/hide

comment:20 Changed 7 years ago by Keith Brown

  • Status changed from reopened to inprogress

comment:21 Changed 7 years ago by Keith Brown

Refs #9043 Added button and widened choose columns

The main gui now has a button that launches the choose columns dialog.

Choose columns now responds to Ctrl-M

The choose columns dialog has been widened, hopefully this should look better on mac now.

Changeset: 47f0be378798c83a69b44e3fd76fe04e4cacbcf9

comment:22 Changed 7 years ago by Keith Brown

Refs #9043 Fixed some comparisons to improve robustness

I believe that the problem that owen mentioned was due to some of my column checking comparisons, so i've fixed them to save out of bounds errors as well as makign it more robust against unforseen problems.

Changeset: dd05c0c4ea36a101fe74f320b7b037af2b73b186

comment:23 Changed 7 years ago by Keith Brown

Even before making that last commit with the comparison fixes, I can't reproduce the paste bug Owen experienced, it was either due to those comparisons, or due to the different line breaks on Mac/Linux. However I can't understand it as when I check for line breaks I'm looking for a "\n" character which should resolve as the host system's line break character(s)

comment:24 Changed 7 years ago by Keith Brown

Refs #9043 Attempt at converting EOL characters cross platform

The pasted string is now put through three conv erters looking for different line ending patterns.

Changeset: 0db570d4bac05f67bb90dec16f200b19ccacb97b

comment:25 Changed 7 years ago by Keith Brown

Refs #9043 Dealing with line endings differently

I've been told about os.linesep so i'm using that instead, as well as suing a different way of checking for empty rows.

Changeset: 306f72129b5f2e091199d3bdc0276a7b46733272

comment:26 Changed 7 years ago by Keith Brown

Refs #9043 Solved EOL problem and fixed an out of bounds error

The EOL problem seems to have been solved by str.splitlines()

Made a mistake when optimizing the checks, it caused and out of bounds error. Solved by merging it with the next check and putting it at the end so that it's the last check to be done

Changeset: 9cb76c26ea4426af4e1778939ec4e1e6589b312e

comment:27 Changed 7 years ago by Keith Brown

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:28 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester changed from Owen Arnold to Martyn Gigg

comment:29 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to reopened
  • Resolution fixed deleted

The functionality seems to work as advertised.

I think the code in Interface/ui/reflectometer/refl_gui.py could use a little tidy up though. A few things:

  • There are no spaces between any of the method definitions
  • There are virtually no comments and any commenting seems to be done with Python docstrings rather than the comment character #. The proper place for docstrings is immediately after the def line and the standard is to use double quotes, i.e.
        def on_comboPolarCorr_activated(self):
        """
            Event handler for polarisation correction selection.
        """
            if self.current_instrument in self.polarisation_instruments:
                chosen_method = self.comboPolarCorrect.currentText()
                self.current_polarisation_method = self.polarisation_options[chosen_method]
            else:
                logger.notice("Polarisation correction is not supported on " + str(self.current_instrument))
    

rather than what is currently on line 88.

  • There seems to be a lot of hard-coded numbers dotted over the code that refer to column indexes. Can these be factored out to separate constants so that the information is in a single place.
  • Both the copy_cells & paste_cells methods are quite long, can they be split up easily?

comment:30 Changed 7 years ago by Keith Brown

The whitespace issue is dealt with in #9170, which is one of the tickets done the further down chain of blocked tickets when i did a general tidy-up of the code

Believe it or not, that docstring on ln 88 was done by Owen in ff81622e180dda8f78a09cbd6d956ed3409971e3, when he did it i assumed that was the right way from then on. I'll fix these in #9170 as that had a lot of refactoring done in it.

I suppose those column indexes could be factored out.

I'll look at the copy/paste method splitting, it may be possible but i'll have to juggle around some of the limit calculations

comment:31 Changed 7 years ago by Martyn Gigg

I assumed the blockers had not been started yet so after some discussion we have decided to leave all refactoring until #9170 to try and minimize merge conflicts.

comment:32 Changed 7 years ago by Keith Brown

  • Status changed from reopened to verify
  • Resolution set to fixed

comment:33 Changed 7 years ago by Martyn Gigg

  • Status changed from verify to verifying

comment:34 Changed 7 years ago by Martyn Gigg

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/9043_Refl_gui_Excel_style'

Full changeset: a4dd979fa76de67251c58c082a355ecd562d976a

comment:35 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 9886

Note: See TracTickets for help on using tickets.