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

Improve Mesh layer description v2 #3572

Merged
merged 2 commits into from
Mar 27, 2019
Merged

Improve Mesh layer description v2 #3572

merged 2 commits into from
Mar 27, 2019

Conversation

rosaguilar
Copy link
Contributor

@rosaguilar rosaguilar commented Mar 19, 2019

Description

Goal:
To adjust the text to include suggestions and subsections (supersedes #3565)

Ticket: fix #2908 fix #2943 #2967 #2752

  • [ x] Backport to LTR documentation is required

Minimal requirements for merging (for maintainers)

  • The description of this PR is coherent with the manual and does not provide wrong information.
  • This PR passes the checks.

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 20, 2019

@rosaguilar there does not seem to be a difference between this pull request and #3565 (same branch with same commits so will call same comments). To address the comments in #3565, you have to edit the files in your branch and create a new commit that (if pushed online) will automatically update the PR. you don't need a new PR unless it' for another issue (this pr should be closed).

If you need more help on how to make changes on an existing pull request, you may give a look to our step by step guidelines (https://docs.qgis.org/testing/en/docs/documentation_guidelines/first_contribution.html#make-corrections). This one focuses on changes in Github but the idea is the same if you do your changes locally and push you branch to the web. Or just ask; i think we have enough git guru around to help you go through this.

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 20, 2019

@rosaguilar there does not seem to be a difference between this pull request and #3565 (same branch with same commits so will call same comments). To address the comments in #3565, you have to edit the files in your branch and create a new commit that (if pushed online) will automatically update the PR. you don't need a new PR unless it' for another issue (this pr should be closed).

If you need more help on how to make changes on an existing pull request, you may give a look to our step by step guidelines (https://docs.qgis.org/testing/en/docs/documentation_guidelines/first_contribution.html#make-corrections). This one focuses on changes in Github but the idea is the same if you do your changes locally and push your branch to the web. Or just ask; i think we have enough git guru around to help you go through this.

Hi, The idea was to add the subsections as you suggested and remove unused icons. Also, to request backport to LTR. Clearly, something went wrong. Sorry about that. I will back to this later.
@DelazJ I need to fetch the changes and upload the changes I did locally. Some guidance will be appreciated.

@rosaguilar
Copy link
Contributor Author

@DelazJ @havatv I have finished and push my changes. Would you like to take a look?

Copy link
Contributor

@havatv havatv left a comment

Choose a reason for hiding this comment

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

Some further suggestions.

Copy link
Collaborator

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

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

@rosaguilar Sorry for the delay, I had reviewed a bit and forgot to publish my comments. So here they are:
As this PR replaces the previous one, it could be nice to fix the issues mentioned there (thanks for having implemented some already), that is:

  • removing the change to conf.py file and remove the make.bat file if not desired
  • remove addition of the icons files that are already available in the repository (minus, plus, symbology, tbutton)
  • (this was not said and I think it might be worth a clear addition to our guidelines, unless @rduivenvoorde changes folder and build structure in the meantime 😃 ) Move the other icons (contours_active, contours_inactive, rendering (see below), groups_exploring (?), vector_active, vector_inactive) from the relative ./img folder to the /static/common folder (where all icons are placed). Only screenshots are expected to be in the relative img folder.
  • rename the rendering.png file. we already have one in the static/common folder. Maybe rendering_mesh (or what's used in the QGIS code repo if different)

I'm not a mesh user so I discover it somehow through your PR. That means I can't comment in details what is expected or help you explain some features but reading it helps me a lot to understand what is behind. Thanks and good job.

@DelazJ DelazJ changed the title 'meshlayerwork' Improve Mesh layer description v2 Mar 25, 2019
@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

@rosaguilar Looks like you removed some icons instead of

Move the other icons (contours_active, contours_inactive, rendering (see below), groups_exploring (?), vector_active, vector_inactive) from the relative ./img folder to the /static/common folder (where all icons are placed). Only screenshots are expected to be in the relative img folder.

You should indeed place the missing icons you are adding in the /static/common/ folder and (I also forgot the last time, sorry) reference them in the /source/substitutions.txt file.
Ideally, they would need to be also mentioned in the source/docs/documentation_guidelines/substitutions.rst so that they are displayed here and prevent people trying to add them again. But this is something we can also take care of in a later step.

@rosaguilar
Copy link
Contributor Author

@rosaguilar Looks like you removed some icons instead of

Move the other icons (contours_active, contours_inactive, rendering (see below), groups_exploring (?), vector_active, vector_inactive) from the relative ./img folder to the /static/common folder (where all icons are placed). Only screenshots are expected to be in the relative img folder.

You should indeed place the missing icons you are adding in the /static/common/ folder and (I also forgot the last time, sorry) reference them in the /source/substitutions.txt file.
Ideally, they would need to be also mentioned in the source/docs/documentation_guidelines/substitutions.rst so that they are displayed here and prevent people trying to add them again. But this is something we can also take care of in a later step.

Hi, I followed your suggestions and did the substitutions.
I tried to moved, but for some reasons I am not getting them in the static/common

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

I tried to moved, but for some reasons I am not getting them in the static/common

Can you elaborate, please ? How did you try the move?

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 26, 2019

I tried to moved, but for some reasons I am not getting them in the static/common

Can you elaborate, please ? How did you try the move?

I commented the .gitignore to track my local additions (i added the small icons to common/static).
I am doing again, to see what was wrong.

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

Your files are placed in source/static/common instead of static/common

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 26, 2019

Your files are placed in source/static/common instead of static/common

Oh... sorry. I will fix it. thanks!

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

Do you have a local repo you are using or are you doing all your changes through github? I'm particularly interested in how you made 1079647

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

Ah I see you upload them to the right place. Don't forget to drop the source/static folder

@rosaguilar
Copy link
Contributor Author

Do you have a local repo you are using or are you doing all your changes through github? I'm particularly interested in how you made 1079647

As you may see, it has been a bit chaotic..sorry for that

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

As you may see, it has been a bit chaotic..sorry for that

That's not an issue. It can be cleaned afterwards. I'd say, the most important is to understand the logic behind and why it's failing.

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 26, 2019

OK. Travis CI is happy now 😃 ! And the PR is in a rather good shape (the remaining points can be handled in another step)
In order to clean the history of your modifications, can you reply to my previous question:

Do you have a local repo you are using or are you doing all your changes through github (web interface)?

I understood you were on Windows: are you also using eg the Github Desktop software? In which case, using menus-only or git command line tools?

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 26, 2019

OK. Travis CI is happy now 😃 ! And the PR is in a rather good shape (the remaining points can be handled in another step)
In order to clean the history of your modifications, can you reply to my previous question:

Do you have a local repo you are using or are you doing all your changes through github (web interface)?

I understood you were on Windows: are you also using eg the Github Desktop software? In which case, using menus-only or git command line tools?
I was using git command line tools basically.
Nice that Travis is happy. I am happy too.
Thanks for all the support.

@rosaguilar
Copy link
Contributor Author

Your files are placed in source/static/common instead of static/common

Thanks!.

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 26, 2019

Do you have a local repo you are using or are you doing all your changes through github? I'm particularly interested in how you made 1079647

Thanks to @rduivenvoorde I got a venv where I could build myself the local repo (in that way was easy for me to see the results e.g., rst labels.. etc). I used git command line tools to update changes from my local repo ... but after some comments/suggestions, commits, etc. It became messy. For example, I tried to commit suggestions that were outdated, and I was a bit confused about what I should or not to do. I needed extra help to synchronize everything again (locally), after commit some local changes I decided to continue editing in github and synchronizing just partially in my local repo.

Deleting files 1079647 was manually in the github online. It was the faster way. Sorry for the several commits.

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 27, 2019

Below a recipe (from a self-taught and somehow git beginner) that might work to squash all these commits in a single one, assuming that git remote -v returns origin for your online repository and upstream for the qgis/QGIS-Documentation's

# update your master branch with the one at qgis repo, by first fetching the upstream data
git checkout master
git fetch upstream master
# pull them in your master branch. I notice that your online master branch is derived from qgis/master but if it's not the case for the local master branch then this should work
git pull upstream master
# otherwise use the one below
git reset --hard upstream/master
# then push the changes online
git push -f origin master

In github.com, your master should be now inline with qgis/master, and same for the one you locally have.
side note: This is my preferred way to update my master branch; I don't like using git merge to pull the upstream modifications in my branch.
Now let's apply the changes in this PR on top of the master branch

git checkout 'meshlayerwork'
git rebase master
#  here you may get two conflicts in mesh_properties file: open it in your text editor and fix them
# (you'd want to remove the HEAD part in both cases)
#  once everything is fixed
git add source/docs/user_manual/working_with_mesh/mesh_properties.rst
git rebase --continue
#  again another conflict, in the same file with again removing HEAD part
git add source/docs/user_manual/working_with_mesh/mesh_properties.rst
git rebase --continue
#  then the rest should process smoothly

Now you have your changes in your local branch on top of the master branch
Let's squash the about forty commits

git reset master

This brings the head commit same as the one of master and, on top of it you have all your changes in an unmerged state. You can then choose the files to add (I don't know whether the gitignore or the make.bat files should be commited - I let more skilled people comment that). Otherwise

git add .
git commit -m "put here your commit message"
# let's update the pull request by pushing the changes online
git push -f origin 'meshlayerwork'

And you should be done.
Hope that helps and hope that works.

@rosaguilar
Copy link
Contributor Author

Below a recipe (from a self-taught and somehow git beginner) that might work to squash all these commits in a single one, assuming that git remote -v returns origin for your online repository and upstream for the qgis/QGIS-Documentation's

# update your master branch with the one at qgis repo, by first fetching the upstream data
git checkout master
git fetch upstream master
# pull them in your master branch. I notice that your online master branch is derived from qgis/master but if it's not the case for the local master branch then this should work
git pull upstream master
# otherwise use the one below
git reset --hard upstream/master
# then push the changes online
git push -f origin master

In github.com, your master should be now inline with qgis/master, and same for the one you locally have.
side note: This is my preferred way to update my master branch; I don't like using git merge to pull the upstream modifications in my branch.
Now let's apply the changes in this PR on top of the master branch

git checkout 'meshlayerwork'
git rebase master
#  here you may get two conflicts in mesh_properties file: open it in your text editor and fix them
# (you'd want to remove the HEAD part in both cases)
#  once everything is fixed
git add source/docs/user_manual/working_with_mesh/mesh_properties.rst
git rebase --continue
#  again another conflict, in the same file with again removing HEAD part
git add source/docs/user_manual/working_with_mesh/mesh_properties.rst
git rebase --continue
#  then the rest should process smoothly

Now you have your changes in your local branch on top of the master branch
Let's squash the about forty commits

git reset master

This brings the head commit same as the one of master and, on top of it you have all your changes in an unmerged state. You can then choose the files to add (I don't know whether the gitignore or the make.bat files should be commited - I let more skilled people comment that). Otherwise

git add .
git commit -m "put here your commit message"
# let's update the pull request by pushing the changes online
git push -f origin 'meshlayerwork'

And you should be done.
Hope that helps and hope that works.

@DelazJ Thanks a lot :)

@DelazJ
Copy link
Collaborator

DelazJ commented Mar 27, 2019

Glad it works.
Because I really would like to have this automagically backported to 3.4 (and some would say i'm a nitpicker 😄 - but do not listen to them), can you

git checkout 'meshlayerwork'
git reset --hard upstream/master
git cherry-pick 6d7b3ba
git cherry-pick 1f73b5e
git push -f origin 'meshlayerwork'

This will erase the two merge commits that are of master modifications and couldn't be backported without manual changes to 3.4. Thanks.

@rosaguilar
Copy link
Contributor Author

rosaguilar commented Mar 27, 2019

Glad it works.
Because I really would like to have this automagically backported to 3.4 (and some would say i'm a nitpicker 😄 - but do not listen to them), can you

git checkout 'meshlayerwork'
git reset --hard upstream/master
git cherry-pick 6d7b3ba
git cherry-pick 1f73b5e
git push -f origin 'meshlayerwork'

This will erase the two merge commits that are of master modifications and couldn't be backported without manual changes to 3.4. Thanks.

Done!. Let's see if Travis is happy now.

@DelazJ DelazJ merged commit 501aa3d into qgis:master Mar 27, 2019
@DelazJ
Copy link
Collaborator

DelazJ commented Mar 27, 2019

Thanks @rosaguilar . Great work!

@backporting
Copy link

backporting bot commented Mar 27, 2019

The backport to release_3.4 failed:

Commits ["33b6cbc8b2f67b4e7f25a55a981681a6b3717c9d","c0d61a1696a1aa276fb23dcc1cc45ac8daa6c174"] could not be cherry-picked on top of release_3.4

To backport manually, run these commands in your terminal:

# Switch to the desired base branch.
git checkout release_3.4
# Update it to its latest state from GitHub.
git pull --rebase
# Cherry-pick all the commits of this pull request.
git cherry-pick 33b6cbc8b2f67b4e7f25a55a981681a6b3717c9d c0d61a1696a1aa276fb23dcc1cc45ac8daa6c174
# Create a new branch with these backported commits.
git checkout -b backport-3572-on-release_3.4
# Push it to GitHub.
git push --set-upstream origin backport-3572-on-release_3.4

Then, create a pull request where the base branch is release_3.4 and the compare/head branch is backport-3572-on-release_3.4.

@rosaguilar
Copy link
Contributor Author

Thanks @rosaguilar . Great work!

Thanks to everyone that helped!

@saberraz
Copy link
Contributor

Thanks a lot @rosaguilar from myself and my colleagues. It's been an enormous task.

@ghtmtt
Copy link
Contributor

ghtmtt commented Mar 28, 2019

thanks @rosaguilar for this work! Nice to see new people working on the documentation. You've picked up a super huge topic for the first contribution :) Thanks also to all the people that helped with suggestions and reviews!

@rosaguilar rosaguilar deleted the 'meshlayerwork' branch March 29, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants