Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add data dialog option to tool form data selector #7553

Merged
merged 40 commits into from
Apr 15, 2019

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Mar 18, 2019

This PR adds the data dialog option to data selectors in the tool and workflow execution forms. The dialog can be opened by clicking the button to the right of the corresponding select field. The data dialog supports the selection of single and multiple history datasets and of datasets from within collections. Similar to the drag-and-drop feature data types are not filtered in the data dialog, however invalid/questionable selections are labeled as 'Unavailable' followed by the dataset name. Additionally, data types are displayed in the data dialog.

image

let Galaxy = getGalaxyInstance();
var self = this;
let self = this;
let galaxy = getGalaxyInstance();
Copy link
Member

@dannon dannon Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pick a standard and follow it here. The rest of the codebase uses Galaxy to refer to the singleton object. I suppose an argument could be made that it should be lowercased since it's not a constructor, but I'm kinda partial to Galaxy myself, though maybe that's just historical familiarity on my part. Anyone else have feelings on this?

Copy link
Contributor Author

@guerler guerler Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that it is lower-case now indicating a local variable. Gives some peace of mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's the rub, right? It's a local reference to a singleton, there's one Galaxy object for the entire app. I do slightly prefer Galaxy to indicate that it's not just a throwaway local object, but we should decide this as a group and stick with it instead of just changing one at a time, here and there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I noticed that we recently started to switch to the lower-case those I followed up with it on any new code. Eitherway is fine with me. Once we have some consensus we should stick to it.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 21, 2019

Thanks for working on this @guerler. I guess the same concerns come up that were already discussed in #5384 and #740, and we have a way to extract single datasets now using Extract Dataset from a list.

In addition to what is discussed in those issue I don't think an additional button is a good choice because

  • there is yet another button that alters the input behavior
  • I don't think this can work for nested collections (as opposed to say a drill-down as proposed by @erasche)

What do you think about a new data_ref style parameter that lets you select collection elements ?
This could then be used in the filter collection tool and the extract element tools or any tool that would benefit from such a mode of selecting data. This would be less awkward than uploading a text file or entering the element identifier by hand, would be traceable and extractable into a workflow.

It is of course not as tightly integrated with the tool form, but I think that's a good thing, because you would very rarely ever need this button, so there's less room for confusion / one less button to click.

@jxtx
Copy link
Contributor

jxtx commented Mar 21, 2019

@mvdbeek The goal of the dataset picker is not just to extract from collections, it is to select a dataset from anywhere in Galaxy -- another history, a library, etc. So I think there needs to be some way to get at it. There may be ways to streamline the UI, but we need something.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 21, 2019

That's a great plan, getting input from libraries and other histories will be a cool feature that would warrant this button and I don't see a problem with that at all. Are we sure we want to do this also for collections ?

@mvdbeek
Copy link
Member

mvdbeek commented Mar 21, 2019

Just gave it a try and I do like the button, and it also solves the recursion issue already, so that's not a valid concern.
Screen Shot 2019-03-21 at 14 46 20

Copy link
Member

@martenson martenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When selecting a dataset for fastqc, the dropdown will freeze and JS error is fired. Note that I have added tags on my history datasets. When I don't have tags this works as expected.

ui-select-default.js?293c:214 Uncaught TypeError: utils_utils__WEBPACK_IMPORTED_MODULE_3__.default.generateTagStyle is not a function
    at eval (ui-select-default.js?293c:214)
    at reducer (underscore.js?17fb:224)
    at Function.eval (underscore.js?17fb:231)
    at Object.formatResult (ui-select-default.js?293c:208)
    at populate (select2.js?4de7:968)
    at constructor.populateResults (select2.js?4de7:992)
    at constructor.eval (select2.js?4de7:1809)
    at Object.eval [as callback] (select2.js?4de7:684)
    at Object.query (ui-select-default.js?293c:196)
    at constructor.updateResults (select2.js?4de7:1762)
    at constructor.opening (select2.js?4de7:2018)
    at constructor.open (select2.js?4de7:1365)
    at constructor.eval (select2.js?4de7:2224)
    at HTMLAnchorElement.eval (select2.js?4de7:684)
    at HTMLAnchorElement.dispatch (jquery.js?97a9:4737)
    at HTMLAnchorElement.$event.dispatch (jquery.event.drag.js?8719:380)
    at HTMLAnchorElement.$event.dispatch (jquery.event.drag.js?aa31:377)
    at HTMLAnchorElement.elemData.handle (jquery.js?97a9:4549)

picker1

@dannon
Copy link
Member

dannon commented Apr 10, 2019

@guerler the bug @martenson mentions is the same as another one I looked at in the past.

@guerler
Copy link
Contributor Author

guerler commented Apr 12, 2019

@martenson this issue has been fixed in dev and I merged it into this PR. Thanks @dannon.

Copy link
Member

@martenson martenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At my request the new data dialog has been hidden from the tool form for the 19.05 release but is still available for the visualizations which was one of the main goals of this PR afaik.

I will create a followup issue with further discussion and ux suggestions.

Thanks @guerler !

@guerler
Copy link
Contributor Author

guerler commented Apr 15, 2019

Thanks for the review @martenson. As discussed in Loray, I disabled the data dialog option in this PR, so we can separately discuss what to improve to get this feature is ready for release. We can discuss this in #7746 which makes the data dialog option visible in the tool form. The follow-up also fixes the job preparation for library dataset by extending the library dataset support in the tool framework. The data tool parameter is now able to interpret 'ldda' as source, just like 'hda' and 'hdca'. It works well and I will add more tests to #7746. I appreciate your suggestions on improving the data dialog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants