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

Observing lists fix #2553

Merged
merged 42 commits into from
Sep 22, 2022
Merged

Observing lists fix #2553

merged 42 commits into from
Sep 22, 2022

Conversation

Jocelyn1109
Copy link
Contributor

@Jocelyn1109 Jocelyn1109 commented Aug 4, 2022

Fixes #2525
Fixes #2411
Fixes #1944
Fixes #1941
Fixes #1933
Fixes #1932
Fixes #1884
Fixes #976

Jocelyn added 26 commits August 2, 2021 13:21
when name of the list is empty, now, save/close button is disabled
- modification for pull request
- deletion of empty item in lists combobox
- new way for signal/slot connection
- adding error message in case of list name duplication,
- loading default list whenn opening obsListDialog,
- sort list name in combo box.
Fix merge master into ObservingLists-fix
Correction of the sort list in combo
adding legacy bookmarks into observing list
Sorting objects when the list is loaded.
- The key defaultlist doesn’t appear in the observingList file,
- The list is not loaded in the tree view, when there is no default list,
- Lost of existing sort when we modify a list.
- The key defaultlist doesn’t appear in the observingList file,
- The list is not loaded in the tree view, when there is no default list,
- Lost of existing sort when we modify a list.
When we delete a list, now we have a confirmation popup.
clear highlight after delete a list, default uuid, etc...
Multiple corrections and modifications
Correction of wrong jd (lost of accuracy)
Adding JD and Location checkbox in obsListDialog.
Use new method getObjectType().
@github-actions
Copy link

github-actions bot commented Aug 4, 2022

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

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files

Magnitude and name correction
@lgtm-com
Copy link

lgtm-com bot commented Sep 10, 2022

This pull request fixes 3 alerts when merging 21ecb8e into 0697699 - view on LGTM.com

fixed alerts:

  • 2 for Comparison result is always the same
  • 1 for FIXME comment

@alex-w
Copy link
Member

alex-w commented Sep 12, 2022

I think the changes in the code is ready to public testing

@alex-w alex-w linked an issue Sep 12, 2022 that may be closed by this pull request
@alex-w alex-w linked an issue Sep 12, 2022 that may be closed by this pull request
@gzotti
Copy link
Member

gzotti commented Sep 12, 2022

I have a few entries for the Moon, stored with date/location. In the bookmark list, all show the coordinates of the "current" moon. Would it not be better to store&retrieve also coordinates, or at least recreate the coordinates from the stored time&locations?

When a bookmark was stored, it apparently stored the location name, but no other data. Sometimes this location comes from a "landscape". On bookmark loading, users are thrown to long/lat 0/0. If location was not found, it is better to not move to the ocean around the equator.

On the plus side, I have so far not triggered a crash :-)

@Jocelyn1109
Copy link
Contributor Author

Hi, @gzotti is it possible to have your observinglist.json ?
I would like to analyze and understand what you want . :)

Many thanks.

Jocelyn

@gzotti
Copy link
Member

gzotti commented Sep 12, 2022

OK, here. (Renamed for Github)

observingList.json.txt

I see when location is empty, the user is moved to 0/0. It would be better to not move location in this case. Probably also locations only retrieved from the landscapes are lost on storing when there is no coordinates for the Landscape placename stored in the location database. It may be useful to explore storing the whole location (name, coordinates, timezone, LP info, ...) into a string which could be retrieved by Location::createFromLine(). For this, developing a fitting Location::toString() would be required. In any case, if no numerical data can be found for a location, the position should not change to just 0/0. (Note somebody may store 0/0 explicitly! Then it is required to go there...)

@gzotti
Copy link
Member

gzotti commented Sep 13, 2022

Of course StelLocation::serializeToLine() is the inverse of createFromLine() which you can use to encode all location data into a single entry for the JSON.

@Jocelyn1109
Copy link
Contributor Author

@gzotti
"I have a few entries for the Moon, stored with date/location. In the bookmark list, all show the coordinates of the "current" moon. Would it not be better to store&retrieve also coordinates, or at least recreate the coordinates from the stored time&locations?" -> yes it is absolutely better and I will modify this point :)

@alex-w alex-w added bug Something likely wrong in the code enhancement Improve existing functionality labels Sep 18, 2022
@Jocelyn1109
Copy link
Contributor Author

@alex-w @gzotti
I noticed that at the moment we forbid having objects with the same name in the list (I think that's what we decided). The control is currently done on the name (designation).
I am going to change this behavior in the following way:
if the object we are adding is already present and the data is identical then there is no point in adding it.
If the object we are adding to the list is already present but RA or DEC, or JD or location are different, then we can add it.
This will allow, for example, to have the moon object but at different observation locations.

Jocelyn

@gzotti
Copy link
Member

gzotti commented Sep 19, 2022

Yes, this is good. Imagine you want to show the eclipsed Moon from various locations at the same time. This should allow demonstrating that it appears equally dark, but in different places (az/alt) in the sky, and esp. in slightly differing places depending on topographic parallax.

As said, please also consider optionally storing the landscape ID for an automated context change.

@Jocelyn1109
Copy link
Contributor Author

@gzotti
the landscape id is already taken into account but optional (checkbox in create dialog).

"observingLists": {
        "{4c48abf7-8ea3-4b06-be90-dc7655b5e2d0}": {
            "creation date": "2022-08-08 07:00:45",
            "description": "location tests",
            "landscape id": "",
            "name": "location test",
            "objects": [
                {
                    "constellation": "CMa",
                    "dec": "-26°23'26\"",
                    "designation": "HIP 34444",
                    "fov": 60,
                    "isVisibleMarker": false,
                    "jd": 2459799.948451157,
                    "location": "Tamiyah, Northern Africa",
                    "magnitude": "1.8",
                    "nameI18n": "Wezen",
                    "objtype": "double star",
                    "ra": "7h08m22.2s",
                    "type": "Star"
                },
            ],
            "sorting": ""
        },

@alex-w
Copy link
Member

alex-w commented Sep 20, 2022

@Jocelyn1109 is it possible pushing the fixes in next 2 days to adding them into RC1?

@Jocelyn1109
Copy link
Contributor Author

Jocelyn1109 commented Sep 20, 2022

@alex-w yes I will do my best :)
But the problem seen by @gzotti is not blocking because it only concerns legacy bookmarks.
For the moment we cannot have in the list (observinglist) two objects with the same name, so the problem will not occur.
I think we can start the test without this fix.

@alex-w
Copy link
Member

alex-w commented Sep 21, 2022

@gzotti is the currently state of the pull request enough to merge and extract the translatable lines to prepare RC1 tomorrow?

@Jocelyn1109
Copy link
Contributor Author

@alex-w @gzotti
Tonight I will push the code that fixes a SIGSEV.
But for the moment the problem of Georg is not corrected because I do not yet know how to retrieve the coordinates (Ra and Dec) of an object with the time and the location.

@gzotti
Copy link
Member

gzotti commented Sep 21, 2022

@alex-w @gzotti Tonight I will push the code that fixes a SIGSEV. But for the moment the problem of Georg is not corrected because I do not yet know how to retrieve the coordinates (Ra and Dec) of an object with the time and the location.

If this is a problem only for the old bookmark lists, don't bother. For new entries it should be enough to store Ra/Dec when the entry is created with timestamp. But it is important to not change location if the location cannot be decoded correctly.

Hmm, location should as discussed be stored as StelLocation::serializeToLine() and decoded with StelLocation::createFromLine(const QString& line); to fully store data in case location comes from a landscape or was manually configured.

The GUI should be edited a bit: move all optionally stored items above or below. Currently checkboxes in the list edit panel to store coordinates and fov are below, landscape is above, and there is no JD entry. In the data retrieval panel, there are only checkboxes for JD and location, no landscapes and no fov.

Please also rebase this branch from current master.

@alex-w alex-w merged commit ef6d37e into Stellarium:master Sep 22, 2022
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 22, 2022
@github-actions
Copy link

Hello @Jocelyn1109!

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

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Oct 1, 2022
@github-actions
Copy link

github-actions bot commented Oct 1, 2022

Hello @Jocelyn1109!

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

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