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 functionality for specify packaged project filename and title #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SeqLaz
Copy link
Member

@SeqLaz SeqLaz commented Oct 10, 2024

This is complement for the PR on QFieldSync

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.

We did interactive review with @SeqLaz

Please fix the tests too

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.

Still not there.

If you accept the comments from the PR, please accept them in batch instead of individual commits.

Also note that the CI is failing. We need to fix this too, don't overlook it.

Finally, we should also add a test about this.

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>

Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>
@SeqLaz SeqLaz force-pushed the functionality_to_specify_filename_and_title branch from 5766a85 to c54da53 Compare October 22, 2024 22:34
@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 22, 2024

Still not there.

If you accept the comments from the PR, please accept them in batch instead of individual commits.

Also note that the CI is failing. We need to fix this too, don't overlook it.

Finally, we should also add a test about this.

I squashed all the commits into only one commit; after all, there was no need for many changes!

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.

Getting there, please add the title thing to the QgisCoreOffliner.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 23, 2024

Getting there, please add the title thing to the QgisCoreOffliner.

Hey @suricactus please have a look.
I added the option to specify the project title passed to the offliner QfisCoreOffliner. And addressed all the recommendations.

Comment on lines 279 to 281
export_project_filename = self._export_filename.parent.joinpath(
f"{self._export_filename.stem}.qgs"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export_project_filename = self._export_filename.parent.joinpath(
f"{self._export_filename.stem}.qgs"
)
export_project_filename = self._export_filename

This is enough, we can add a check earlier that self._export_filename is a .qgs file.

This should also be handled in the UI.

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.

Ok, now it's mostly styling stuff, all the rest looks good !

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 24, 2024

Ok, now it's mostly styling stuff, all the rest looks good !

Hey @suricactus, in this last commit, I addressed all the suggestions. Have a look!

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