gui icon indicating copy to clipboard operation
gui copied to clipboard

List-box class needs rewrite

Open AMDphreak opened this issue 4 years ago • 10 comments

The list-box class needs a re-write in a bad way.

Required fields

image

Instantiating a list-box should not require you to define choices for the list-box. It should be optional, so that the user can populate it when they are ready. This is basic Object-oriented thinking. The list-box could be very useful for presenting search results to a user for selection, but doing so is miserable. For a work-around, I had to use an empty string to initiliaze the choices, keeping the list-box hidden, then use the set method with empty lists (send blah set (list ) (list )) to clear it out before showing the list-box and populating the fields.

Set method

image

Also, the set method is dumb, because it clears the list for no reason. This makes no sense at all. It makes this function completely useless for typical use-cases of list-box.

Append

image

https://docs.racket-lang.org/gui/list-box_.html?q=gui#%28meth._%28%28%28lib._mred%2Fmain..rkt%29._list-box~25%29._append%29%29

Also, the append method does not allow populating all fields when adding a new entry, if there is more than 1 column to populate. This is a weird limitation. As a work-around, I had to first populate the new list item's first column with append and then populate the remainder of the fields using set-string. This is not intuitive at all. Also, the [data] field is confusing. People who are making GUIs will read that to mean "the data I want displayed will go here". This is a bad name for that field. In this context, you cannot use "data" to describe something that is not visible. Try something unused like "arb" You can define an arb data type as "arbitrary data" in another doc entry. This will resolve the conflict between the user's definition of "data" and Racket's definition.

The right way:

  • Make choices optional.
  • Get rid of the set function (replaced by append below)
  • Provide essential methods:
    • append
      (send a-list-box append (list* field1 field2 field3 ...) [(list* arb1 arb2 arb3 ...)]
      
      Creates a new row. Takes a list of strings and a list of arbitraries. Assigns each string to a corresponding field in the row.
      (send a-list-box append
                      (list* (list* field1 field2 field3 ...)
                             (list* field1 field2 field3 ...)
                             .
                             .
                             .
                             )
                      [(list* (list* arb1 arb2 arb3 ...)
                              (list* arb1 arb2 arb3 ...)
                              .
                              .
                              .
                              )])
      
      Appends a list of string-lists (a 2D array), so the user can add n new entries to the list.
    • insert
      (send a-list-box insert (blah))
      
      where blah is the same kind of structure shown above in append.
    • clear - clear the contents.
      (send a-list-box clear)
      

AMDphreak avatar Nov 16 '19 08:11 AMDphreak

I noticed there is a clear function linked in the list-box% Docs, but it only shows up in the list on the left. It does not show up anywhere in the actual function definitions.

AMDphreak avatar Nov 16 '19 08:11 AMDphreak

It seems to be you could write a PR based on this design.

mfelleisen avatar Nov 16 '19 14:11 mfelleisen

I agree. I think that it also makes sense to make another class if backward compatibility cannot be maintained for old code that already uses those methods.

rfindler avatar Nov 16 '19 16:11 rfindler

Actually I thought of something else. Maybe I can get rid of the insert method and the append method and simply use an add method.

(send a-list-box add
                (list* (list* field1 field2 field3 ...)
                       (list* field1 field2 field3 ...)
                       .
                       .
                       .
                       )
                [(list* (list* arb1 arb2 arb3 ...)
                        (list* arb1 arb2 arb3 ...)
                        .
                        .
                        .
                        )
                 start-position])

Adding is commutative so it covers both the cases where we are appending and inserting at a position or index. I am not sure how this would behave with the current implementation of list-box, if something is added while the user is clicking on the columns to reorder the list. Ideally the program would add new items to an immutable list (internal to the list-box object and not directly visible), and this list would be transformed into the requested ordering (column and increasing or decreasing). This would require a separate list just for the requested ordering. If there is only one list, then the program would end up showing new items in wrong places. However, if there is an internal list/external list implementation, then we don't need to worry about the start-position and can take it out of the method signature altogether.

It seems to be you could write a PR based on this design.

If I can figure it out I will. Not the biggest brain around.

I agree. I think that it also makes sense to make another class if backward compatibility cannot be maintained for old code that already uses those methods.

This is a good point. But we also have versioning to help warn people about breaking changes (lol, major version increment for something as stupid as this...what am I thinking). If we are snifty enough, we can simply implement the old functions via the new functions and deprecate it in the docs.

AMDphreak avatar Nov 18 '19 10:11 AMDphreak

I can actually write an imperative version of this but haven't looked at the source enough to see if we have to write this in a functional sytle.

(define (real-append list-box input)
  (define acc 0)
  ;; Outer loop
  (for-each (lambda (x) 
              ;; Imperative work.
              (begin 
                (send list-box append (first x))
                
                ;; Inner loop 
                (for-each
                 (let ([inner-acc 1])  
                   (lambda (y) (begin 
                                 (send list-box set-string acc y inner-acc)
                                 (set! inner-acc (+ 1 inner-acc)))))
                 (rest x))
                ;; Next list. 
                (set! acc (+ 1 acc))))
            
            input))

I was thinking something like this, although it is extremely imperative it would do what @AMDphreak is suggesting as I am using it in my own program right now. I might be able to translate this into two for/folds if I really sat down and threw my brain at it.

MarkusReynolds1989 avatar Jul 06 '21 18:07 MarkusReynolds1989

Some time ago I wrote a GUI control on top of list-box% to make it more convenient when used to display data with lots of columns and rows. You can use it directly, or as an inspiration on how to write your own GUI control op top of list-box%: https://github.com/alex-hhh/qresults-list

alex-hhh avatar Jul 07 '21 00:07 alex-hhh

@alex-hhh I might confer to your code if I attempt to rewrite this method or add it! Appreciate you posting this.

MarkusReynolds1989 avatar Jul 07 '21 01:07 MarkusReynolds1989

I strongly agree with the initial assessment. List-box% is awkward to use. The two things that I really do not understand are:

  • why do we need to feed the data to a multi-column listbox column by column? Who does this? In every single GUI framework I've ever used, you feed the data row by row, as a list of items.
  • why is the set function variadic? Why not simply take a list of lists as a single argument? Now I have to jump through hoops to 1) transpose the data so it's fed column by column, and 2) make up some crazy contraption to feed as many arguments as there are columns in my data. For a 2-column data-set, it's relatively simple:
(let ((transposed-contents (transpose initial-listbox-contents)))
        (send new-listbox set
              (first transposed-contents)
              (second transposed-contents)))

...but what if I want to handle any number of columns? Now I have to use apply to pass arguments from one variadic function to another. It's all possible, but it could be so much simpler. +1 to rewriting this.

DexterLagan avatar Feb 01 '22 08:02 DexterLagan

@DexterLagan Leaving aside the broader questions here, your code snippet can be written as

(send/apply new-listbox set (transpose initial-listbox-contents))

samth avatar Feb 01 '22 15:02 samth

Aaaah yes send/apply! I forgot all about this. Thank you!

DexterLagan avatar Feb 01 '22 15:02 DexterLagan