Conversation
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
|
So the comments about R And have been addressed now. But how can I reach |
|
BTW, there's also another change that I introduced in some modules: the use of a |
b4a2935 to
dffab61
Compare
This method gets called by the Search dialog from its fourth tab, Lists. |
|
I didn’t checked the code yet, but what about changes for list of latest founded objects in search tool? |
What changes? The type is saved to the config file and loaded from there. |
|
Hmm, Peacock is broken... |
|
Actually, the whole search mechanism is broken: even in |
But maybe you're expanded the feature? ;) P.S. I checked the branch yesterday as an user and it works better than master |
No, I haven't expanded it. The issue with Peacock appeared now because of the order of results that doesn't play well with the last stored names in the Search Dialog's name-type map. Previously this wasn't a problem because whatever was found by name was both the item that was selected on goToObject, and the one whose type was displayed. Now these data are separate. The proper solution is to completely redo this module-type feature, saving a list of name-type pairs instead of a map into the JSON, and storing it likewise in the C++ code. |
|
There appear to be two different object "types": one returned by I've rewritten the logic of the search, so that there's only one method that actually searches for an object and selects it, and both kinds of type are now stored, including in the recent object history. |
|
One small issue now is that, when we go from the list of objects, user-visible type is not registered in the Recents. I'll fix that later. |
|
So far all the issues I had identified are fixed. The legacy Recents list is now ignored, only the new-format entries will be loaded. I think it's not too much of an issue. |
The Search dialog is currently implemented in a very suboptimal way, especially since #4635, when it started triggering searches by name in its CompletionListModel::data method. The search is supposed to get object type for each item found, which is only a name of the object. What the dialog would really want is to have object pointers in the search results, not simply names. This requires a huge refactoring of the search API in StelObjectModule, so that the object listing methods return the object pointers. This is exactly what this change does. In addition to the pointers the API still returns the names, because it looks like the right names for search results are not always trivial to find out from the object itself. Fixes #4655.
f094e78 to
52472dd
Compare
alex-w
left a comment
There was a problem hiding this comment.
Thanks for refactoring the feature
The Search dialog is currently implemented in a very suboptimal way, especially since #4635, when it started triggering searches by name in its
CompletionListModel::datamethod. The search is supposed to get object type for each item found, which is only a name of the object.What the dialog would really want is to have object pointers in the search results, not simply names. This requires a huge refactoring of the search API in
StelObjectModule, so that the list-object methods return the object pointer. This is exactly what this change does. In addition to the pointers the API still returns the names, because it looks like the right names for search results are not always trivial to find out.Now, I needed to split the finding of the double and variable stars to initialization stage, because otherwise it'd have to be done synchronously during a search, which is not nice. It appears that
R Anddoesn't get found easily and somehow requires a search for a Gaia object, which then stumbles upon the fact thatStarMgris initialized beforeStelSkyDrawer, while such a deeper search requiresStelSkyDrawer. I've replaced theR Anddesignation withHIP 1901(in b166597), but I'd like to know if any of the other stars could face a similar problem in some circumstances. Maybe we should simply change all the designations in the list to catalog numbers for ease of search.I'm leaving the
searchByNameandsearchByNameI18ncalls inSearchDialog.cppfor now, but hopefully it won't be required anymore.I'm marking this PR as a draft because it's the first appearance of this huge change, but I think it's fairly complete and ready for reviewing.
Fixes #4655