Fix: unified ObservingLists dialog by gzotti · Pull Request #2954 · Stellarium/stellarium

gzotti · 2022-12-29T16:13:16Z

Description

The state of ObservingLists in 1.0 is functionally OK, but IMHO still not user friendly.

Editing opens a secondary dialog. I fail to see the point.

It is also not maintainer friendly

needlessly long variable names that mostly start with the same name.
inconsistent variable names
lots of duplicate code
The handling of the lists requires repeated loading and looping through the lists.

It should be enough to load the lists once and manipulate the copy kept in memory. Just occasionally, we need to store the list. Maybe even only write on exit? Probably a copy of the existing list should be added on loading, in case of program failure.

Fixes #2802 (issue)

Screenshots (if appropriate):

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
This change requires a documentation update

How Has This Been Tested?

Test Configuration:

Operating system: Win11, Qt6.4
Graphics Card: (irrelevant)

Checklist:

My code follows the code style of this project.
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (header file)
I have updated the respective chapter in the Stellarium User Guide
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

github-actions · 2022-12-29T16:14:15Z

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

Are possibly unused includes removed?

Files matching guide/**:

Did you remember to update screenshots to match new updates?
Did you remember to grammar check in changed part of documentation?

This is an automatically generated QA checklist based on modified files

gzotti · 2022-12-31T19:02:11Z

@alex-w there is a mismatch in ObsListDialog LL 454ff vs. LL600ff when assigning type and objtype. I am not sure what the type/objtype now really should resemble. It is really problematic to cross-assign as is now done in LL454f. I also don't know why we store these in the JSON in the first place.

This may also relate to #2802. The ObsList has entries "designation" and "nameI18n". I would never store an internationalized name in a list that we also redesigned so that users can exchange lists across (language) borders.

Shall we develop format version 2.1 or (3.0): store "designation" and "name" (i.e., englishName), and find an i18n on the fly just for displaying? Yet another issue is star and DSO names: these are sky culture dependent. So, it may be OK to just use designation, and deal with a "name" entry for the human reader of the JSON file, but load/overwrite the name during loading from the designation.

A small issue is the switching between normal and edit mode: I would like to hide the horizontal layouts. Those cannot be hidden, so I hide the buttons instead. If all buttons in a horizontalLayout are hidden, it vanishes. But I don't know how to accomplish that if there is a spacer included.

gzotti · 2023-01-28T20:18:53Z

@alex-w can you please look into these questions?

alex-w · 2023-01-31T03:54:29Z

@gzotti sorry for late answer.

type/objtype is important to storing into JSON for correct finding of the object - for example B 10 can be deep-sky object and star. Localized name can be removed from JSON and we can storing into JSON designations of objects only (probably except the DSO without designations). English and localized names may be found on the fly.

gzotti · 2023-01-31T10:35:38Z

Yes, but I think it is inconsistent between importing old bookmarks and storing new objects! Or is it working correctly as it is now?
I would store (one most common) englishName into the JSON for reference (for the people editing the JSON manually!), but on loading we better retrieve the name from the currently active list.

alex-w · 2023-01-31T10:45:40Z

Yes, but I think it is inconsistent between importing old bookmarks and storing new objects! Or is it working correctly as it is now? I would store (one most common) englishName into the JSON for reference (for the people editing the JSON manually!), but on loading we better retrieve the name from the currently active list.

Yeah, but in any way current observing lists are not compatible with old bookmarks

gzotti · 2023-01-31T11:19:51Z

Yes, the JSON formats are not the same, I know. I just saw that when loading a list there is a flip between objType and type. See ObsListDialog lines 452ff and compare to lines 607ff when importing bookmarks. Is this correct? It at least requires a note in the source code that whoever developed this originally knew what they were doing. My "Caveat" comment is that of a code reviewer indicating a possible source of error.

alex-w · 2023-01-31T12:07:57Z

Yes, the JSON formats are not the same, I know. I just saw that when loading a list there is a flip between objType and type. See ObsListDialog lines 452ff and compare to lines 607ff when importing bookmarks. Is this correct? It at least requires a note in the source code that whoever developed this originally knew what they were doing. My "Caveat" comment is that of a code reviewer indicating a possible source of error.

Well, we should 2 important features:
a) the tool for import/export observing lists with some specific extension - .solist for example (Stellarium Observing List)
b) the tool for import bookmarks - a tool for converting bookmarks into observing lists (remember - the bookmarks has extension .json)

The current OL feature has a partially implemented of both tools.

gzotti · 2023-01-31T12:25:28Z

Currently both list formats have the too generic json extension, but importing the old format and adding missing data works well. However, this does not answer my question about the type/objType issue. The new format (without I18n names) could be named .ol3.

The major task of this PR was to remove the separate edit dialog and the repeated need to load the JSON file, and making the code more readable. I think the current state is feature-equivalent w.r.t. the JSON files, i.e. has the same errors in data conversion. Now we should try to remove the errors in the conversions and file format. If this warrants a new format (version 3) with e.g. .ol3 ending, it's OK for me.

alex-w · 2023-01-31T12:38:49Z

Currently both list formats have the too generic json extension, but importing the old format and adding missing data works well. However, this does not answer my question about the type/objType issue. The new format (without I18n names) could be named .ol3.

The type option was introduced in first implementation of OL and this is my fault - it was not complete, because it has stored wrong data by the fact. To resolve the problem and for backward compatibility the objtype was introduced later. Of course the type of object should be found on the fly. I think we should store one parameter for definition of type of object and this parameter should had data for the correct finding of the object (see second parameter for StelObjectMgr::searchByName() method)

The major task of this PR was to remove the separate edit dialog and the repeated need to load the JSON file, and making the code more readable. I think the current state is feature-equivalent w.r.t. the JSON files, i.e. has the same errors in data conversion. Now we should try to remove the errors in the conversions and file format. If this warrants a new format (version 3) with e.g. .ol3 ending, it's OK for me.

.ol3 is OK for me too

alex-w · 2023-01-31T12:44:29Z

Oops, sorry, I forgot the very important note - by the fact we should have 2 data files: a) observingList.json (maybe observingList.dat) which should contains collection of all created/imported observing lists in current instance of Stellarium and b) somename.ol3 with single observing list as file for exchange the data!

gzotti · 2023-02-01T21:39:14Z

The current import can read old bookmarks.json format and new obslist.json formats. We can change or limit this to .ol3 files (new format only), but this is another PR. I am not sure after the month of waiting, but I think you can also import another obslist file (with several obslists) and import all lists.

This still does not solve the type issue/bug. The PR is a branch in the main repo. Do you have time to take over and do what has to be done? I think the type/objType should be fixed first. If this requires a different file format again, this should be a well-designed one-last-time step, and then should not be changed. But I think it is not required to remain compatible with the 1.0-1.2 format 2.0 which effectively will have been used only for 6 months.

- unified GUI (old ui files will be removed later) - moved methods from CreateEdit class to standard class - simplified common/util class (will be removed later) Todo: - Load JSON once - Work with sub-lists in memory - Write JSON on exit

- clearer variable names - start avoiding needless file IO - restructure GUI - obsolete edit dialog ready for removal Caveat: Many things don't work correctly now!

- UI elements renamed (final?) - removed excessive writing - removed old files - start testing

Avoid a type not found-error However, there is a mismatch between bookmarks and obsList handling!

- Also describe the current problem...

gzotti · 2023-03-05T17:05:55Z

@alex-w please review this. As far as I can see it is feature equivalent to the old version, but has one dialog less. However, the type/objType mess is still here to be cleared up. Further, I18N names should be loaded on the fly, and never stored to (or at least never read from) JSON. New file endings can be introduced later.

alex-w · 2023-03-06T03:34:52Z

@alex-w please review this. As far as I can see it is feature equivalent to the old version, but has one dialog less. However, the type/objType mess is still here to be cleared up. Further, I18N names should be loaded on the fly, and never stored to (or at least never read from) JSON. New file endings can be introduced later.

I tried run updated OL without old data - neither bookmarks or lists - and I got the crash. But I did not reproduced the crash later.

After adding few objects into list I see something like:

                {
                    "constellation": "Sct",
                    "dec": "-6°16'18\"",
                    "designation": "Wild Duck Cluster",
                    "fov": 0,
                    "isVisibleMarker": true,
                    "jd": 0,
                    "landscapeID": "",
                    "location": "",
                    "magnitude": "5.80",
                    "nameI18n": "Скопление Дикая утка",
                    "objtype": "рассеянное звёздное скопление",
                    "ra": "18h51m04.4s",
                    "type": "Nebula"
                },
                {
                    "constellation": "Aql",
                    "dec": "-0°49'25\"",
                    "designation": "HIP 99473",
                    "fov": 0,
                    "isVisibleMarker": true,
                    "jd": 0,
                    "landscapeID": "",
                    "location": "",
                    "magnitude": "3.20",
                    "nameI18n": "Альмизан III",
                    "objtype": "двойная звезда",
                    "ra": "20h11m17.3s",
                    "type": "Star"
                },

I agree - no need saving nameI18n field into JSON - it should be obtain on-the-fly. Probably similar behaviour should be for objtype field or, at least, it should be English only. I see some inconsistense for DSO objects - why not use designations for it?
Like:

QString dsoName = object->getDSODesignation();
if (dsoName.isEmpty())
	dsoName = object->getDSODesignationWIC()
if (dsoName.isEmpty())
	dsoName = object->getNameI18n();

P.S. Probably constellation and magnitude fields can be obtain on-the-fly too and in this case we may avoid saving these data.

gzotti · 2023-03-11T15:42:48Z

The objectMap.value(KEY_TYPE) should actually be the object's class name, so that it can be retrieved in the objectMgr.findAndSelect(item.designation, type) call. It can have values like "Planet", "Star", "Nebula", ...
This can be stored in item.type (and I would prefer to rename this to item.class for clarity), but display of this column can be suppressed just like display of objectUUID.

The item.objtype on the other hand could be a temporal variable for display purposes only. Here we may show the various subtypes like "Moon", "Cubewano", "Dark Nebula", "Spiral Galaxy", ... These have to be retrieved on-the-fly when loading the list into the TreeView. It may be allowed to store it into the JSON list (for reference when manually editing data lists later), even in translated form (because it is "write only"), however, it must never be evaluated during loading.

@alex-w do you agree?

And can you please check whether this interpretation of the items is correct? What is the purpose of the isVisibleMarker entry? When is it evaluated? Can it be toggled by the user? Why is that not just an entry with type=Marker?

struct observingListItem
	{
	public:
		QString designation;   //!< Relates to designation in the JSON file and englishName in most classes. DSO 
		QString nameI18n;      //!< Relates to name in the JSON file.
		QString type;          //!< type oriented on Stellarium's object class: Star, Planet (also for sun and moons!), Nebula, ...
		QString objtype;       //!< More physical type description: star (also Sun!), planet, moon, open cluster, galaxy, region in the sky, ...
		QString ra;            //!< Optional, position at data jd; useful for moving objects
		QString dec;           //!< Optional, position at data jd; useful for moving objects
		QString magnitude;     //!< NOT a redundant bit of information! In case of moving objects, it spares computing details on loading.
		QString constellation; //!< NOT a redundant bit of information! In case of moving objects, it spares computing positions on loading.
		double jd;             //!< optional: stores date of observation
		QString location;      //!< optional: should be a full location encoded with StelLocation::serializeToLine()
		QString landscapeID;   //!< optional: landscapeID of landscape at moment of item creation.
		double fov;            //!< optional: Field of view
		bool isVisibleMarker;  //!< WHAT IS THE PURPOSE OF THIS?

I had hoped to get that solved by January 5. To fix that finally still for 23.1, I suggest we concentrate on this one now for the weekend.

alex-w · 2023-03-12T03:29:34Z

The objectMap.value(KEY_TYPE) should actually be the object's class name, so that it can be retrieved in the objectMgr.findAndSelect(item.designation, type) call. It can have values like "Planet", "Star", "Nebula", ... This can be stored in item.type (and I would prefer to rename this to item.class for clarity), but display of this column can be suppressed just like display of objectUUID.

The item.objtype on the other hand could be a temporal variable for display purposes only. Here we may show the various subtypes like "Moon", "Cubewano", "Dark Nebula", "Spiral Galaxy", ... These have to be retrieved on-the-fly when loading the list into the TreeView. It may be allowed to store it into the JSON list (for reference when manually editing data lists later), even in translated form (because it is "write only"), however, it must never be evaluated during loading.

@alex-w do you agree?

Yes, I agree.

And can you please check whether this interpretation of the items is correct? What is the purpose of the isVisibleMarker entry? When is it evaluated? Can it be toggled by the user? Why is that not just an entry with?

Let imagine that you find some object via SIMBAD and this object is not in Stellarium database - it will be added as custom object (type=Marker by the fact, but without visible marker) which you can add into OL (type=Marker, isVisibleMarker=false). Of course you can put markers on the sky and add some of them (or all) into OL (type=Marker, isVisibleMarker=true) too.

struct observingListItem
	{
	public:
		QString designation;   //!< Relates to designation in the JSON file and englishName in most classes. DSO 
		QString nameI18n;      //!< Relates to name in the JSON file.
		QString type;          //!< type oriented on Stellarium's object class: Star, Planet (also for sun and moons!), Nebula, ...
		QString objtype;       //!< More physical type description: star (also Sun!), planet, moon, open cluster, galaxy, region in the sky, ...
		QString ra;            //!< Optional, position at data jd; useful for moving objects
		QString dec;           //!< Optional, position at data jd; useful for moving objects
		QString magnitude;     //!< NOT a redundant bit of information! In case of moving objects, it spares computing details on loading.
		QString constellation; //!< NOT a redundant bit of information! In case of moving objects, it spares computing positions on loading.
		double jd;             //!< optional: stores date of observation
		QString location;      //!< optional: should be a full location encoded with StelLocation::serializeToLine()
		QString landscapeID;   //!< optional: landscapeID of landscape at moment of item creation.
		double fov;            //!< optional: Field of view
		bool isVisibleMarker;  //!< WHAT IS THE PURPOSE OF THIS?

OK

- also make method names clearer - also added some documentation

- renamed a few variables for clarity - now recreating localized names on list loading - Localized name entries stored for external use, but not read back

gzotti · 2023-03-13T00:30:11Z

@alex-w can you repeat your tests please?
I have not touched the isVisibleMarker AFAIK, but my workflows are often different from yours, so don't know if it works different from 1.2.

The current solution allows editing in the same dialog by reconfiguring a few buttons. The currently edited list is stored (replaces the list state of before editing) when 'Save list' is pressed. The list JSON file is written on exiting Stellarium.
When exiting Stellarium during list editing, the current edits are not saved.

I hope this works finally free from further major surprises...

alex-w

Everything what I tested are works as expected

github-actions · 2023-03-13T15:30:40Z

Hello @gzotti!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

github-actions · 2023-03-27T06:48:28Z

Hello @gzotti!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

gzotti added enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash state: in progress The problem is in process of solution... labels Dec 29, 2022

gzotti added this to the 23.1 milestone Dec 29, 2022

gzotti self-assigned this Dec 29, 2022

github-actions Bot requested review from 10110111 and alex-w December 29, 2022 16:13

gzotti force-pushed the fix/Unified_ObsList branch from b59a6a7 to 384787f Compare December 31, 2022 00:40

gzotti marked this pull request as ready for review December 31, 2022 00:41

gzotti mentioned this pull request Feb 7, 2023

PrintSky plugin #1008

Draft

19 tasks

gzotti added 8 commits March 5, 2023 12:43

First changes

78bd623

- unified GUI (old ui files will be removed later) - moved methods from CreateEdit class to standard class - simplified common/util class (will be removed later) Todo: - Load JSON once - Work with sub-lists in memory - Write JSON on exit

ObservingLists: The big rename

22f374c

- clearer variable names - start avoiding needless file IO - restructure GUI - obsolete edit dialog ready for removal Caveat: Many things don't work correctly now!

ObservingLists: Further changes

c5816c0

- UI elements renamed (final?) - removed excessive writing - removed old files - start testing

SUG: small changes on ObservingLists

4ffb416

Ensured file paths are constant

74be4fa

Add a confirmation warning to overwrite an existing list on import

534dcdf

Add missing header

fc15876

Simplified QString constants

e714076

Avoid a type not found-error However, there is a mismatch between bookmarks and obsList handling!

Added more documentation

4dcb521

- Also describe the current problem...

gzotti force-pushed the fix/Unified_ObsList branch from 382a79d to 4dcb521 Compare March 5, 2023 11:43

gzotti added 2 commits March 11, 2023 14:54

Fix QString references

17cbd4a

Fix creation of default empty list at start

0a14018

Docfix

0002527

gzotti added 2 commits March 12, 2023 14:42

remedy a bit of code bloat

8609a78

- also make method names clearer - also added some documentation

Fixed the type/objtype bug

34105bf

- renamed a few variables for clarity - now recreating localized names on list loading - Localized name entries stored for external use, but not read back

alex-w approved these changes Mar 13, 2023

View reviewed changes

Cleanup and docfixes

f19d386

gzotti linked an issue Mar 13, 2023 that may be closed by this pull request

ObserveList and IC1848 Soul Nebula #2802

Closed

Fix storing DSO with designations (Fix #2802)

5b98c17

gzotti merged commit c453545 into master Mar 13, 2023

gzotti deleted the fix/Unified_ObsList branch March 13, 2023 11:20

alex-w added state: published The fix has been published for testing in weekly binary package and removed state: in progress The problem is in process of solution... labels Mar 13, 2023

alex-w removed the state: published The fix has been published for testing in weekly binary package label Mar 27, 2023

Uh oh!

Conversation

gzotti commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots (if appropriate):

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

github-actions Bot commented Dec 29, 2022 • edited by gzotti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Jan 28, 2023

Uh oh!

alex-w commented Jan 31, 2023

Uh oh!

gzotti commented Jan 31, 2023

Uh oh!

alex-w commented Jan 31, 2023

Uh oh!

gzotti commented Jan 31, 2023

Uh oh!

alex-w commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Jan 31, 2023

Uh oh!

alex-w commented Jan 31, 2023

Uh oh!

alex-w commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Feb 1, 2023

Uh oh!

gzotti commented Mar 5, 2023

Uh oh!

alex-w commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Mar 11, 2023

Uh oh!

alex-w commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzotti commented Mar 13, 2023

Uh oh!

alex-w left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 13, 2023

Uh oh!

github-actions Bot commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gzotti commented Dec 29, 2022 •

edited

Loading

github-actions Bot commented Dec 29, 2022 •

edited by gzotti

Loading

gzotti commented Dec 31, 2022 •

edited

Loading

alex-w commented Jan 31, 2023 •

edited

Loading

alex-w commented Jan 31, 2023 •

edited

Loading

alex-w commented Mar 6, 2023 •

edited

Loading

alex-w commented Mar 12, 2023 •

edited

Loading