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

Adding the option for specify the project title on packaged project #615

Merged

Conversation

SeqLaz
Copy link
Member

@SeqLaz SeqLaz commented Sep 25, 2024

This adds the option to specify a new project name and title in the exported project when packaging.

Before After
Before Image After Image

Related PR: opengisch/libqfieldsync#94

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Instead of complicating the code in the caller (in this case QFieldSync), we should add this functionality within the OfflineConverter itself. Why? Because the converter already does the copy of the file.

For example here we have hardcoded the name of the output file: https://github.com/opengisch/libqfieldsync/blob/master/libqfieldsync/offline_converter.py#L276

What we can do is to pass the output name to be a parameter when we construct the OfflineConverter class. We can even replace the export_dir param with export_filename param, and then derive the export_dir to be the path to export_filename.

@suricactus
Copy link
Collaborator

Please note that most of the suggestion here need code-wide changes, the suggestions are only pointing to the highlights.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 10, 2024

Please note that most of the suggestion here need code-wide changes, the suggestions are only pointing to the highlights.

@suricactus The changes and suggestions were addressed in this PR

@suricactus
Copy link
Collaborator

suricactus commented Oct 16, 2024

Few comments after the libqfieldsync review:

  • Drop the "export directory" and export finame in favor of single "export filename" with the full path
  • After clicking "Create" OR when changing the value of "export filename", make sure the user is notified there is already a file with that name in that location.
  • Prepopulate the "export_filename" with the default value of "export directory" with "/" and current project basename and "_qfield" suffix and ".qgs" extension. You can create a utility function in libqfieldsync get_export_filename_suggestion(project.fileName())

Please update the screenshot when ready.

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Not there yet.

@SeqLaz SeqLaz force-pushed the add_text_edit_for_project_title_on_offline_converter branch from 16fe607 to 39ba5d8 Compare October 24, 2024 00:39
@SeqLaz SeqLaz force-pushed the add_text_edit_for_project_title_on_offline_converter branch from 39ba5d8 to 5118656 Compare October 24, 2024 00:47
@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 24, 2024

Not there yet.

Hey @suricactus please have a look.
I squashed due to the PR had a lot of merge. In the last commit I addressed all the suggestion and some improvements.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 24, 2024

Hey @suricactus, I added a message box that informs the user they should use the .qgs extension. Have a look!

@SeqLaz
Copy link
Member Author

SeqLaz commented Nov 1, 2024

Hey @suricactus I added the typing hints on the new functions I added, have a look!

@SeqLaz
Copy link
Member Author

SeqLaz commented Nov 7, 2024

Hey @suricactus, I deleted the unnecessary function and joined the functions of export_folder and get_filename_suggestion.

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Ah, just realized, you never bumped libqfieldsync here. Last step before merging. I guess the tests will fail here too.

@SeqLaz
Copy link
Member Author

SeqLaz commented Nov 13, 2024

Ah, just realized, you never bumped libqfieldsync here. Last step before merging. I guess the tests will fail here too.

Hey @suricactus, yes, you are right. I just added the corresponding libqfieldsync dependency.

@suricactus
Copy link
Collaborator

@SeqLaz you have a conflict now. I will resolve it to unblock the tests.

@SeqLaz
Copy link
Member Author

SeqLaz commented Nov 13, 2024

Hey @suricactus I've fixed the tests!

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Good!

@suricactus suricactus merged commit e51f5c7 into master Nov 14, 2024
7 checks passed
@suricactus suricactus deleted the add_text_edit_for_project_title_on_offline_converter branch November 14, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants