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

Fix observing list import/export #3207

Merged
merged 4 commits into from Sep 12, 2023
Merged

Fix observing list import/export #3207

merged 4 commits into from Sep 12, 2023

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Apr 26, 2023

Fix a few quirks and a real bug in ObsList import and export.

Description

List export exports a single list.
On import, the name of the list is checked.

List date is date of creation, not of last edit. Maybe we should change that?

The oh-so-important OLUD is actually not tested. If a list is created, an OLUD is given. Exporting to file, editing the "live" list, storing and importing the previously exported would (I think) just overwrite the existing list with same OLUD. Is this not what should have been avoided? What is the scenario?

Fixes

  • Importing observe list doesn't replace default observe list #3206 (issue)
  • after loading a list, show it immediately and fill its name in the name combo
  • On export, let users export single lists (ending: .sol) or the whole list of lists .ol
  • Change timestamp from list creation date to date of last edit.
  • On import, make sure to test existing lists not by name and last date, but by OLUD (or all 3?)
  • (other observations)

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
  • 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

@gzotti gzotti added the bug Something likely wrong in the code label Apr 26, 2023
@gzotti gzotti added this to the 23.2 milestone Apr 26, 2023
@gzotti gzotti added this to Needs triage in Observing Lists via automation Apr 26, 2023
@github-actions
Copy link

github-actions bot commented Apr 26, 2023

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.

@alex-w
Copy link
Member

alex-w commented Apr 26, 2023

I didn't check the code yet, but I have important notification for import/export feature. In the original idea the OL is contains many lists of objects with some description and settings, of course, the OLUD is unique identifier for each list. The import/export feature should import/export one list per file and by the fact OL should support 2 formats of files - OL (all lists) and SOL (single observing list - the format for exchange the data). By the fact currently the format of SOL file is wrong. :(

See my observing lists file and exported default list from v23.1:
observingList.json.txt
observingList_default.json.txt

@alex-w alex-w moved this from Needs triage to In progress in Observing Lists Apr 26, 2023
@gzotti
Copy link
Member Author

gzotti commented Apr 26, 2023

The SOL/OL distinction was never discussed before January I think. Can you please edit the PR description with checkable list entries what should be done. And then please help to implement these things, esp. OL/SOL stuff. Not starting on June 15, but this weekend, finish by ~May 20 to allow real beta testing before release. I have never seriously used the lists in the new format, so some issues evaded my attention.

I just see the danger written above that the whole OLUD idea is useless if it allows edit/export/import without giving a new OLUD. Probably a new OLUD should be given whenever content changes (basically, create a checksum from list contents: entries, name, date), so that each import can immediately see whether this list exists already of if it was changed. We could then as well use the list name as key (instead of OLUD), plus a checksum entry for that purpose of identification. (Format 3.0...)

@ilkant
Copy link

ilkant commented Apr 28, 2023

I think that two timestaps to observe lists are good. Creation timestamp and last edited timestamp. Like Linux files.

@alex-w
Copy link
Member

alex-w commented Apr 28, 2023

The SOL/OL distinction was never discussed before January I think. Can you please edit the PR description with checkable list entries what should be done.

I should find all my record for this feature before editing checklist...

And then please help to implement these things, esp. OL/SOL stuff. Not starting on June 15, but this weekend, finish by ~May 20 to allow real beta testing before release.

Good point

I have never seriously used the lists in the new format, so some issues evaded my attention.

Same problem

I just see the danger written above that the whole OLUD idea is useless if it allows edit/export/import without giving a new OLUD. Probably a new OLUD should be given whenever content changes (basically, create a checksum from list contents: entries, name, date), so that each import can immediately see whether this list exists already of if it was changed. We could then as well use the list name as key (instead of OLUD), plus a checksum entry for that purpose of identification. (Format 3.0...)

No. The OLUD should be unique within one collection of observing lists, but we've never guaranteed unique OLUD for whole world span. I hope in some time later on our website will be added new category for user contributed data - favorites/observing lists with good description and interesting data.

@gzotti
Copy link
Member Author

gzotti commented Apr 28, 2023

Is the OLUD ever checked? Or will importing a renamed list with same OLUD overwrite an existing list? The latter case must be avoided, else the whole idea is broken! So, at least when importing, the check must be extended to not just name, but also OLUD.

@ilkant
Copy link

ilkant commented Apr 28, 2023

I have a vision: Observe list could be a set of objects. And then there can be a script which creates a star trek using the observation list. And the user can share and receive lists of observation points from others.

I have also thought that in those previous situations it is not necessary that all the information about the target has been stored in the list. However, if the data is in Stellarium data. But if that list is a list of the user's own observations, then there should be places for all the information obtained from the observation.

@gzotti
Copy link
Member Author

gzotti commented Apr 28, 2023

These lists retrieve data from the program. Some people seem to have interest in using these lists externally.

Storing one's own observation notes is not intended at this stage. Years ago, an observation logger was allegedly planned, but without an actual usage scenario apparently went nowhere. I have read about a 3rd-party library/format/spec, but others may be more in actual "observing" than me.

@alex-w
Copy link
Member

alex-w commented Apr 28, 2023

Is the OLUD ever checked? Or will importing a renamed list with same OLUD overwrite an existing list?

Well, as far as I remember the process of importing list into collection should add new list - not just adding some elements into existing one. Of course at this step OLUD is checked and the list will not import if collection have data with same OLUD.

The latter case must be avoided, else the whole idea is broken! So, at least when importing, the check must be extended to not just name, but also OLUD.

I fear in some chunk of refactoring (or probably someone requested it) the behaviour was changed to allow the ability of merging the lists.

@alex-w
Copy link
Member

alex-w commented Apr 28, 2023

I have a vision: Observe list could be a set of objects. And then there can be a script which creates a star trek using the observation list. And the user can share and receive lists of observation points from others.

I have also thought that in those previous situations it is not necessary that all the information about the target has been stored in the list. However, if the data is in Stellarium data. But if that list is a list of the user's own observations, then there should be places for all the information obtained from the observation.

The observing log is different task and it is not in aim of observing lists. The observing lists by the fact is the collection of few bookmarks (favorites) lists.

@alex-w alex-w modified the milestones: 23.2, 23.3 Jun 27, 2023
@gzotti gzotti linked an issue Jul 6, 2023 that may be closed by this pull request
@gzotti gzotti linked an issue Aug 29, 2023 that may be closed by this pull request
@gzotti gzotti mentioned this pull request Sep 12, 2023
13 tasks
- Add last-edited date
- display newly-loaded list (last when loading multiple lists)
- use system time, not simulation time, for list timestamping
- store and load .sol files.
@gzotti
Copy link
Member Author

gzotti commented Sep 12, 2023

@alex-w, Storing (exporting) the whole ObsList as .ol is identical to copying the observingList.json file, right?
What about the file type options? Should I allow import of "*.json, *.ol, *.sol" (do we need to support *.json for legacy, or just skip that?) and, on export, discern "export all" and "export current list" just by the selected extension, one of "*.ol" or "*.sol"?

@alex-w
Copy link
Member

alex-w commented Sep 12, 2023

I guess .sol for single list and .json for legacy/bookmarks. No need "export all" feature IMHO.

- export single .sol and complete .ol lists.
- import single .sol, complete .ol or .json lists.
@gzotti
Copy link
Member Author

gzotti commented Sep 12, 2023

You had expressed earlier that you want OL and SOL. It was not terribly hard to do so, so now it's done. A .sol is just a one-list .ol, and .ol has the same format as the observing list flavor of what was by now .json.

I can export and import lists. Hopefully it is done.

@gzotti gzotti marked this pull request as ready for review September 12, 2023 14:16
@gzotti gzotti added enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash state: implemented The enhancement or feature has been implemented labels Sep 12, 2023
@github-actions
Copy link

Hello @gzotti!

The enhancement or feature has been merged into source code and you may test it via building Stellarium from source code or wait the weekly development snapshot...

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

Thanks!

@gzotti gzotti merged commit ff1df3b into master Sep 12, 2023
22 checks passed
Observing Lists automation moved this from In progress to Done Sep 12, 2023
@gzotti gzotti deleted the fix/ObsList-Import-Export branch September 12, 2023 17:34
@gzotti gzotti mentioned this pull request Sep 14, 2023
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 18, 2023
@github-actions
Copy link

Hello @gzotti!

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 Sep 26, 2023
@github-actions
Copy link

Hello @gzotti!

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
Labels
bug Something likely wrong in the code enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash state: implemented The enhancement or feature has been implemented
Projects
Development

Successfully merging this pull request may close these issues.

Can not import bookmark list Importing observe list doesn't replace default observe list
3 participants