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

feat: new icons #3167

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from
Draft

Conversation

pgilfernandez
Copy link
Contributor

I still consider the PR unfinished but I would like to share it with you so that you give me feedback if you feel like it is necessary or to let me know about other icons still missing.
More or less the ones created are:
menu_file
menu_view
menu_canvas
colors

As said, there still some work to do but let me know if I'm not in the right path.

@pgilfernandez
Copy link
Contributor Author

I fixed them all by using tab indentation instead of space ones. The whole file had a mix of them all so I turned into the one that it seems you use in the rest of the project. I hope. it's fine like this.

@rodolforg
Copy link
Contributor

rodolforg commented Aug 8, 2023

Hey, @pgilfernandez !

Can you rebase your branch to up-to-date master branch?
This PR is getting unrelated stuff here.
Then, re-push it to Github.

Something like:

git fetch upstream master:master
git rebase master canvas_menu_icons_2
git push origin --force

But DO NOT TYPE the commands above if you don't understand them! It may screw your git repo and work o.O

Anyway, replace upstream in the command above with the name you gave to synfig remote repo.
The origin remote repository should be replaced with the name you gave to your own repository.

…commands which used old Adwaita generic ones"

This reverts commit 6930ed3.
… which used old Adwaita generic ones, now with correct descriptions
…r, process-stop and view-refresh commands which used old Adwaita generic ones
@pgilfernandez
Copy link
Contributor Author

pgilfernandez commented Aug 9, 2023

Can you rebase your branch to up-to-date master branch?

Sure, I'm not a GIT master and sometimes I mess something up... tell me if now it's fine or it still contains "errors".
When I rebased it it game me a few conflicts, but I'm sure it was me the only ones that edit them...

This PR is getting unrelated stuff here.

Kind of, yes, my part is all about icons but it got longer than expected. If you feel it's not right don't bother telling me to drop this PR and try another one with all the work done at once with the newest master or if you prefer separate them all (per icons or per related icons) in different PRs for better review or understanding.

git fetch upstream master:master
git rebase master canvas_menu_icons_2
git push origin --force

Don't worry, at least I know what did you mean here, actually is what I usually do but I got problems with a rebase, lost the HEAD of my branch and caused some troubles and dirty commits as you saw.

Let me know what you prefer me to do.
The important thing is that the whole Adwaita icons transfer is finished and I'm already focusing in some extra missing icons, in a week or so Synfig will feel like with a new dress ;-)

Thanks

@pgilfernandez
Copy link
Contributor Author

Hi @rodolforg,

I finished the icons update, would you mind having it a look and telling me if I need to do anything else or is it possible to merge (in case you like everything, indeed)?

@pgilfernandez
Copy link
Contributor Author

an overview of the new icons added since the first post:
workspaces
keyframee_panel

I'm not adding old Adwaita ones as we all may know them as "old friends" =p

@rodolforg
Copy link
Contributor

I think you should split this PR in two: one adds new icons, other changes current ones.

@morevnaproject @ice0

@pgilfernandez
Copy link
Contributor Author

@morevnaproject @ice0, do you agree with @rodolforg?
If so I will create 2 new PRs with splitted commits as he suggested

@morevnaproject
Copy link
Member

I am okay with merging everything as single PR. ^__^

@rodolforg
Copy link
Contributor

@morevnaproject what about the new and the changed icons? XD

@pgilfernandez
Copy link
Contributor Author

I finally had time to split this PR into "feat" and "refactor".
I have just sent the "refactor" one #3196 as I think it makes sense to start with it and when, only when, it is approved and merged then, with the most recent master of that moment, I will create the other PR with the "feat" new icons.

I guess that when both get merged this one could be just closed =p

Cheers

@pgilfernandez pgilfernandez marked this pull request as draft September 8, 2023 14:47
@morevnaproject
Copy link
Member

@morevnaproject what about the new and the changed icons? XD

I like them all!

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.

None yet

3 participants