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

Entries from requests.txt for which icons exist already #1914

Closed
wants to merge 26 commits into from

Conversation

alchemiker
Copy link
Contributor

@alchemiker alchemiker commented Jan 9, 2024

INFO: I somehow failed at creating a new accurate branch without my previous commits, please discard the commits before 09.01.2024, they are in another pull request of mine already

No new icons, only added those to appfilter from requests that already have an icon in the arcticons base already. Those are often language versions, renamed apps, apps from other places than the Play Store and thus with slightly different app pathes, flavours like "pro" or "lite" with no difference in icons (at least in black-and-white-rendition) - stuff like this.

I made a search between appfilter.xml and requests.txt file to find apps that have the same name of the launch activity. I looked up how the apps are called and if the names were different or something else was suspicious I looked the apps up on the internet, and compared their icons with the ones in the Arcticons app.

So far I came to the letter F (update: L). To be continued... but if you are going to release a new version somewhere soon, you can merge, too.

The aim is that more people see Arcticons Icons on their screen straight upon installation of the icon pack without the need to search for them, and not to waste the time of contributors by designing icons that already exist.

@TotallyAvailable
Copy link
Contributor

TotallyAvailable commented Jan 10, 2024

I do feel like there has to be a better way avoiding a maintenance nightmare given that previously covered Icons become available for request again every time the main activity changes.
While the script appears to fix some of those, I manually fixed quite a few of those previously covered by hand so far as well.

Why ?

  • the person requesting it no longer using the app or Icon pack
  • the contributor no longer using the app or Icon pack
  • (FDroid) changed package name for reproducible builds
  • introduction of different flavors
  • forks
  • ...
  • request limit: those with a unique set of Apps are probably targeting "non mainstream" ones first
  • region locking
  • confusing listings: not every Store listed Icon is actually the one you get after installing the App
  • "someone is probably going to fix it"
  • manual assignment "good enough"
  • Apps not listed on F-Droid/Play Store

@alchemiker
Copy link
Contributor Author

@TotallyAvailable Yeah, I think we must find a way to minimize the workload for the long-term, but I think we should rather open up a thread in the discussions pane for this issue. I'm just going to work through my personal list in this pull-request.

We have to find a solution that works for both Donno and the icon creators.

@Donnnno
Copy link
Collaborator

Donnnno commented Jan 11, 2024

Wow, thanks for going through the requests!
Is it possible to rebase and add all the appfilter changes, I can't merge it at this stage, because of conflicts sadly.

@alchemiker
Copy link
Contributor Author

@Donnnno how do I do this? I edited the file in GitHub here in the browser...

@TotallyAvailable
Copy link
Contributor

TotallyAvailable commented Jan 11, 2024

As #1902 isn't merged yet probably https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github

Also appears that several of the currently open PRs have merge conflicts.

@alchemiker
Copy link
Contributor Author

I did delete all the svgs, but there is still a problem apparently. Do I have to go through the appfilter file and delete my 28 entries from the other commit? Or make a fresh new updated branch and add my 100+ commits from from this branch to it? 🤔 I was hoping that Git could help it.
@TotallyAvailable that's something only people with write access to the directory here can do.

@TotallyAvailable

This comment was marked as resolved.

@petlyh
Copy link
Contributor

petlyh commented Jan 12, 2024

@alchemiker I cloned your branch and did a local merge using git rebase. I can't push the merged branch since I don't have write access, but these are the appfilter entries that are causing the conflict:

  • Domino's
  • Google WebView DevTools
  • Recorder

Undoing just your changes to those entries should allow an automatic merge if you want to avoid merging on the command-line. You can restore the icon files since those aren't causing the conflict.

@Donnnno Donnnno force-pushed the alchemiker-doubles branch from 5c95962 to 0237258 Compare January 13, 2024 13:36
Removed my added entries for alts of Domino's, Google WebView DevTools and Recorder in an attempt to resolve the merge conflict
Donnnno added a commit that referenced this pull request Jan 13, 2024
Co-Authored-By: alchemiker <[email protected]>
@Donnnno
Copy link
Collaborator

Donnnno commented Jan 13, 2024

oof, lots of merge errors.

I've added all your changes in a newer commit.

I'd suggest using different branches for different PRs, that avoids merging errors like this.

Also GitHub desktop is really easy to use too :)

@Donnnno Donnnno closed this Jan 13, 2024
@alchemiker
Copy link
Contributor Author

Thank you very much for your patience, this whole story was definitely a lesson for me to make new branches in the future 😅 I am very sorry for all this mess! I'll try to make it better next time.
Do I still have to do something with this branch or is everything finished here now?

@alchemiker alchemiker deleted the alchemiker-doubles branch January 13, 2024 15:22
@Donnnno
Copy link
Collaborator

Donnnno commented Jan 13, 2024

No worries, I also made a mess of it with my attempt to rebase 😆.
I think you can delete this one now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants