JAL-1648: Remember search queries employed previously in Find and Select By...

Activity

CR-JAL-32 14

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author 2h 44m 5 I have revised implementation for this, check if you are ...
    Reviewer - Complete 1h 51m 9 The usual solution to this type of problem would be to us...
    Total   4h 35m 14  
    #permalink

    Objectives

    Find and other free-text dialogs like the Select by.. dialog will be more useful if there was a way of choosing from a previously executed search strategy.

    e.g. change the query field to additionally provide a drop-down menu allowing the user to select from the previous N-th find operations.

    Ideally, if the user picks a previous query, then any associated search settings (e.g. Match Case, Search Description, etc) should also be re-instated.

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Kira Mourão

    There should be unit tests for AppCache.java and UniprotFTSPanel.java.

    There should be unit tests for AppCache.java and UniprotFTSPanel.java.

    Tochukwu Charles N Ofoegbu

    Unit tests on the way.

    Unit tests on the way.

    Kira Mourão

    There is very little in the way of comments explaining what the code is doing...

    There is very little in the way of comments explaining what the code is doing/intended use. This particularly applies to the new class/interfaces in io/cache but the other code would also benefit from more commenting.

    Kira Mourão

    I'm not very comfortable with the design and use of the CacheBoxI interface. ...

    I'm not very comfortable with the design and use of the CacheBoxI interface. For example, in the AppCache::updateCache method, this interface supports calls to set/lose focus on the combobox in the UI. This is unarguably GUI logic and should be controlled in the respective GUI classes, not by the AppCache. It also seems there is already the possibility to do so by incorporating this logic into the existing updateCache methods in the GUI classes. Similarly much of the other logic (particularly in updateCache) could also be dealt with in the GUI code. Doing this is likely to reduce the amount of information the AppCache needs to know about combo box in the UI. In a similar vein, it's not a good idea to name the method to get the information from the combo box as getCacheComboBox because it won't necessarily always be a combo box.

    I see that the motivation for the current code setup might be to put code common to all 3 GUI panel classes (GFTSPanel, PDBFTSPanel, UniprotFTSPanel) in one place, but putting GUI logic into the AppCache class is not a good solution.

    Tochukwu Charles N Ofoegbu

    The motivation for AppCache design is to have the cache implementation logic ...

    The motivation for AppCache design is to have the cache implementation logic in one central place rather than re-implementing them for all instances where caching is required for a text input. I personally think that having separate cache logic implementation for all the various occasions would introduce a maintenance nightmare. I reckon that the trade off of having UI logic (which is abstracted by an interface) in the AppCache outweighs the cost of multiple re-implementation.

    Regarding your question on the CacheBoxI, the first implementation of AppCache was operating directly on the concrete ComboBox object. However, Mungo suggested that I create a layer of abstraction for the ComboBox, hence the reason for introducing CacheBoxI interface.

    "it's not a good idea to name the method to get the information from the combo box as getCacheComboBox because it won't necessarily always be a combo box."
    Here It will always be a combo box by design. You can see that the method returns CacheBoxI (which is an abstraction for combo box). Please see method comment in the Cacheable Interface. Also all text boxes requiring caching were converted to a combo box to enable the dropdown input feature.

    Kira Mourão

    I'm definitely not suggesting that we should have separate cache logic implem...

    I'm definitely not suggesting that we should have separate cache logic implementation for different occasions. It is possible to set this code up so that the UI logic remains in the UI code and the cache logic implementation occurs once. Here is a more specific suggestion on how to do this:
    You already have the GFTSPanel and GFinder implementing Cacheable. You could use this more effectively by making Cacheable an abstract class and pushing the more UI-specific code into Cacheable. A prime candidate for this, as I noted above, is the updateCache method currently in AppCache: the indication that this would be more appropriate in Cacheable is in the number of calls to the cacheable object, and the cacheComboBox object obtained from the cacheable object, which means that the cacheable object's class actually has most of the information to carry out the processing required here.

    I agree that it makes sense to abstract the ComboBox . But note that the current implementation of this abstraction means that there are two sets of near-duplicate code implementing the CacheBoxI interface, in the GFTSPanel and GFinder. Future clients would also have to repeat this implementation. This suggests a design issue.

    I was talking about the getCacheComboBox in the Cacheable interface and its use in AppCache::updateCache: it seems unreasonable to me that the AppCache should need to obtain information from the cacheable object by getting hold of its combobox. I think it would be more appropriate to obtain that information via methods which did not require a combobox to be returned. Additionally it is rather limiting that the AppCache will only work with a combo box, as it seems likely that other UI components might also need to use the AppCache.

    Tochukwu Charles N Ofoegbu

    Making Cacheable an abstract class doesn't seem much like an option here. In ...

    Making Cacheable an abstract class doesn't seem much like an option here. In fact, my initial design thought was to implement a base class for Cacheable interface and have the UI logic implemented there. However, Java does not support multiple inheritance (most of the classes that require caching already extends other classes). Hence, the AppCache design was the best alternative I could think of at that point.

    I agree that the abstraction for ComboBox seems like near-duplicate code. However, they are light weight and anonymous instantiation code required to materialise the unique ComboBoxI interface instance for each Cacheable implementation. This abstracts the CacheBox UI implementation from the cache implementation (if the UI technology changes, the cache implementation wouldn't need to change).

    By the current design, any class that implements Cacheable must also implement and provide an Instance of CacheBoxI. It is necessary for AppCache to access some of the information for a given CacheBoxI instance. The rationale is to provide a handle to manipulate the state / data for the materialised cache combo box for each class which implements Cacheable (I am open to ideas for design improvement). The AppCache was designed specifically to work with combobox for textual user inputs. Most other components that require caching can already be stored in jalview properties via the Cache class.

    Kira Mourão

    The usual solution to this type of problem would be to use some sort of wrapp...

    The usual solution to this type of problem would be to use some sort of wrapper class (e.g. decorator but there are other options) either around the JPanel or the combo box. This avoids the multiple inheritance problem.

    The AppCache only actually needs the user input from the combo box, and as a string this is agnostic of whether the holding class is a combo box or something else. The AppCache also needs the cache key, but that's it. It is the design which requires the AppCache to need the remaining information from the combo box, which it then uses to update the combo box and set focus, functionality which would be better delegated to the combo box, as it is (1) GUI-related and (2) specific to combo boxes. The AppCache need only supply the set of cache items to the combobox.

    You say the rationale is to provide a handle to manipulate the state / data for the materialised cache combo box. I think it is inappropriate for the AppCache to manipulate the state of the materialised cache combo box. This is a GUI issue and you cannot predict what requirements future AppCache clients may have. Manipulating the data is exactly what the AppCache is intended to do, but for that, as I said above, only the user input is needed from the combobox, and only the set of cached items is needed to supply to the combobox.

    A design change to use a wrapper solution would support moving the combobox-specific update out of the AppCache. I'm happy to elaborate if required - it is not a big code change, more just shifting code between classes.

    Tochukwu Charles N Ofoegbu

    I have revised implementation for this, check if you are ok with the improvem...

    I have revised implementation for this, check if you are ok with the improvement. Further comments available on the Jira issue

    /src/jalview/fts/api/GFTSPanelI.java Changed
    /src/jalview/fts/core/GFTSPanel.java Changed 1
    /src/jalview/.../service/pdb/PDBFTSPanel.java Changed
    /src/jalview/.../uniprot/UniprotFTSPanel.java Changed
    /src/jalview/gui/Finder.java Changed
    /src/jalview/io/cache/AppCache.java Added 1
    /src/jalview/io/cache/AppCacheI.java Added
    /src/jalview/io/cache/CacheBoxI.java Added 2
    Open in IDE #permalink
    /src/jalview/io/cache/Cacheable.java Added
    /src/jalview/jbgui/GFinder.java Changed 1
    /test/jalview/.../pdb/PDBFTSPanelTest.java Changed
    Open in IDE #permalink

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against