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

Added icons for set square and compass #4422

Merged
merged 3 commits into from
Jan 2, 2023
Merged

Added icons for set square and compass #4422

merged 3 commits into from
Jan 2, 2023

Conversation

dreng
Copy link
Contributor

@dreng dreng commented Nov 16, 2022

During my tests of the "set square" feature, which is in an early developing stage, I noticed that the icons are barely visible. While discussing some improvements here with Roland I decided to create some new icons.

@rolandlo
Copy link
Member

Thanks @dreng for putting your efforts into this. The icons look pretty good and fit very nicely with the existing icon themes.
We may have to wait though until the Compass is merged or at least until its design is final.

@Technius Technius added this to the v1.2.0 milestone Dec 27, 2022
@rolandlo
Copy link
Member

rolandlo commented Jan 1, 2023

@dreng The compass is merged now. Can you rebase on master and solve the merge conflict? Let me know if you need help.
Moreover the design of the compass has changed slightly (color of the marked radius from green to blue). So maybe make all 3 radii blue (not just 2 of them).

@dreng
Copy link
Contributor Author

dreng commented Jan 1, 2023

@rolandlo Some help is indeed needed here. If I try to sync the branch in my fork, it says that my commit will be discarded. No chance to priorize one of the conflicting files. I can just use Github's editor in order to resolve the confict text-wise, but this is not what I want. I want to preserve my file. The Github docs didn't help either but left me more insecure than before.

@rolandlo
Copy link
Member

rolandlo commented Jan 1, 2023

@rolandlo Some help is indeed needed here. If I try to sync the branch in my fork, it says that my commit will be discarded. No chance to priorize one of the conflicting files. I can just use Github's editor in order to resolve the confict text-wise, but this is not what I want. I want to preserve my file. The Github docs didn't help either but left me more insecure than before.

One way to achieve the goal (which doesn't require solving conflicts):

  1. Make a backup of all the icons in some folder, which is not contained in the root folder (xournalpp) of the repository
  2. Use
git reset --hard upstream/master
  1. Add back the icons to the correct folders and the copyright notice (which you can copy from here), add and commit them
  2. Check the log and commit info with
git log
git show

to see if everything is alright
5) Use

git push --force

to push the changes to Github.

@rolandlo
Copy link
Member

rolandlo commented Jan 1, 2023

@rolandlo Some help is indeed needed here. If I try to sync the branch in my fork, it says that my commit will be discarded. No chance to priorize one of the conflicting files. I can just use Github's editor in order to resolve the confict text-wise, but this is not what I want. I want to preserve my file. The Github docs didn't help either but left me more insecure than before.

One way to achieve the goal (which doesn't require solving conflicts):

  • Make a backup of all the icons in some folder, which is not contained in the root folder (xournalpp) of the repository
  • Use
git reset --hard upstream/master

Here I am assuming that upstream is the remote name for the url https://github.com/xournalpp/xournalpp.git

  • Add back the icons to the correct folders and the copyright notice (which you can copy from here), add and commit them
  • Check the log and commit info with
git log
git show

to see if everything is alright

  • Use
git push --force

to push the changes to Github.

@dreng dreng closed this Jan 1, 2023
@rolandlo
Copy link
Member

rolandlo commented Jan 1, 2023

Looks like there was some problem. There is a shorter way though, that doesn't require backing files up and adding them manually:

git checkout set-square-icons
git reset --hard 593572ee4
git rebase -X theirs upstream/master
git push --force
  • The first command checks out your feature branch that adds the icons
  • The second command goes back to the state you were before trying to rebase on current master.
  • The third command rebases on upstream/master keeping the changes from your feature branch for the merge conflict.
  • The forth command pushes the changes forcefully to Github

@rolandlo rolandlo reopened this Jan 1, 2023
@dreng
Copy link
Contributor Author

dreng commented Jan 1, 2023

Not exactly the way I wanted to go but copying the needed files and re-adding them solved the problem at least.

Git reset --hard somehow didn't work for me. I updated my fork which deleted the file as it stated before. I then rebased the branch and added the previously copied files. Finally I pushed the branch back to my fork.

@rolandlo
Copy link
Member

rolandlo commented Jan 1, 2023

Great you figured out a way! There is one issue left: Please delete the two old setsquare and compass icons from the folder /ui/iconsColor-light/scalabel/actions/. Otherwise in the light color theme the old icons are shown.

@dreng
Copy link
Contributor Author

dreng commented Jan 1, 2023

Overlooked the old icons. Thanks for watching out.

Copy link
Member

@rolandlo rolandlo left a comment

Choose a reason for hiding this comment

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

LGTM. I like the new icons. Definitely an improvement over the old ones.
Merging in 24 hours if no objections are raised.

@rolandlo rolandlo added the merge proposed Merge was proposed by maintainer label Jan 1, 2023
@rolandlo rolandlo merged commit 2459548 into xournalpp:master Jan 2, 2023
@rolandlo
Copy link
Member

rolandlo commented Jan 2, 2023

Thanks again for the icons @dreng ! Just merged them.

@dreng
Copy link
Contributor Author

dreng commented Jan 2, 2023

You're welcome, @rolandlo !

@dreng dreng deleted the set-square-icons branch January 2, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge proposed Merge was proposed by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants