-
Notifications
You must be signed in to change notification settings - Fork 63
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
Future #71
Comments
I've never used Objective-C and Swift seems to be the way things are moving, so my vote would be to switch completely to Swift. I like all the changes from #56 and I would be happy to implement them in Swift. |
Hey, I'd be happy to implement my changes in Swift too 😆 |
;D But yeah, haven't checked out the Swift side to be honest. If it doesn't build on latest Xcode/Swift, make sure it does, then divide up work on porting 56 or something? |
Sweet, thanks guys. Would the two of you be interested in becoming collaborators? I'd give you guys push access. I'd still want pull requests as opposed to directly pushing to the repo just for the sake of code reviews, but I'd trust you guys to merge in code as necessary. |
@sebj Yeah that makes a lot more sense. Let me know if you want any help with that. Seems like #56 should be merged into the 0.5.x for it's functionality and closure, but I have no idea what that entails. Not sure I could be any help there. I thought I got Swift working with #70 (it was locally) but the CI failed |
Sweet, done. You guys should have push access. Only other thing which I should probably mention - the 0.5.X branch was MIT licensed. The Swift branch is in the public domain (via The Unlicense). |
I'm not opposed to merging in #56 for closure. I think that ideally, going forward, the
Would that make sense? Or would it just cause more confusion? |
@citruspi Ooh that's a good idea if we can do that imo |
@sebj FWIW I did run your branch when you originally submitted and I believe I noticed a small bug. I can double check when I've got some free time, but I believe that the value of the Notifications checkbox wasn't saved to user preferences so if a user disabled notifications and restarted the application, it would be re-enabled. |
@citruspi Thanks for the invite! And good luck with the new place and job! So who wants to do:
|
@nbgraham Thanks! I've also just got a new laptop, so I don't have Xcode installed or setup or anything. I've started downloading it, I just want to confirm that I can build and run #56 and everything works before it gets merged in so that we can create a new release. Assuming everything works, I can drop a comment in the thread for #56. I'd probably say that whoever merges in #56 can also rename the branches. However, we can also rename the branches now, prior to merging in #56. I believe that there's an option to edit the branch the pull request is being merged into, so if we rename Any preferences? |
@citruspi Can we merge before renaming? Feels easier/safer in my eyes lol. And on that bug, you're right – just recreated it. After we merge I'll commit again with a fix? |
@citruspi Yep sure. Could we.. enact this repo plan from tomorrow? (Sorry!) I'm pretty exhausted and want to sleep at the moment lol. Would rather approach this bright-eyed tomorrow morning (UK time). |
Sure thing. Once you commit the fix, feel free to merge the pull request and rename the branches. I'm on the US East Coast so if you wait for me, it may be a couple hours after your morning. |
@citruspi Ok, sounds good! |
Seems like I don't have anything to do! Any thoughts on #59? |
@nbgraham I haven't had a chance to take a in-depth look at that pull request, but it looks mostly fine after a cursory glance. I've added a comment about some code which doesn't seem to be used. I also haven't had a chance to try building and running or confirming that it does indeed fix, but based on the code and the comments in #38 and #69 it looks like it should. However, it's worth noting that #59 does two things:
#56 does, among other things, both of those:
So I'm not sure if it's worth merging in #59 and creating merge conflicts for #56 for something which should be fixed by #56 either way. |
Just my two cents, I'm really glad this project will be getting some love again. Love this application :) |
@citruspi Setup and pushed |
Branches should also be fixed as well now.
|
@citruspi Great :) How do we approach this next? I'm happy to carry on porting from |
Good question. :) So I think there's two action items:
For the Swift port, I'm wondering how much sense it would make to actually create issues for work and assign them to people or to try using GitHub Projects. I've created a project for the port here. |
All sounds good! Seems like you can't tag or mention people in Projects cards, but you can keep track of what's being worked on. Would be cool to use as a basic todo list separate from long-standing issues that exist. |
Yeah, a basic todo list would probably make sense. Perhaps just a new issue with a checklist of items. re: Tagging or mentioning people in project cards, you can add issues to the project if you hit the "Add cards" button on the top right. So I think we'd create issues, assign, mention, etc. in the issue and then track the issue via the project. |
Gotcha, sure thing |
@citruspi How would you feel about switching |
@sebj Woah, I didn't realize that the When you say switching the branches, do you mean making the contents of I just tried building the
on the line
in Side note, could we use pull requests when merging new code (including into the development branch (e.g. Happy New Year! |
@citruspi Happy New Year to you too! 😃 On PRs: I think my New Year's resolution should be to actually use feature branches & PRs more often (sorry!) 😬 I believe I've solved that issue, along with a couple other bugs on the As for the future of the branches, I'm unsure as to whether Based on the progress in |
I'm stepping out right now but I'll try building it again when I get back.
Gotcha, yeah, a switcheroo makes sense.
Yep, go for it. |
@sebj I've pulled down the latest changes and the application now builds but notifications don't appear and the status of Spotify (at the top of the menu) is stuck to |
@citruspi I just made some more changes locally, I think I've fixed that too. 😬 Also may have bundled in changes to prefs: |
Sweet! @ me when you push up the changes and I'll take a look. |
@citruspi Just pushed! |
Just realized that the Last.fm menu item is also disabled. |
@citruspi Argh. I think I've just fixed the problem you mentioned with a very small fix (lemme know). |
@sebj, thanks for fixing #81 and taking a look at the changes in #84. I've been at a wedding for the last couple days, but I'll be back tomorrow and I'll take a look at the state of the For what it's worth, you can probably just close #84. Like you said, some of @JackWa's changes are covered in changes you've made to the Thanks again. |
@citruspi That's fair enough, sounds good to me. Apologies if I caused you to get spammed with Github notifications(!). I've just been trying to clear some of the open issues on a couple of my Github repos recently, hence some activity here. I've closed #84, but looks like my tentative comment on #81 has been proved wrong. Otherwise, #67 is covered, but I've left a line commented out that would remove featured artists from the track name if we've pulled them out for the notification subtitle. |
Yeah, so a couple things.
In the last year or so, I've dropped out of college, gotten a job, moved to a new city, travelled a bunch, etc. It's been a pretty busy year and I haven't been able to give this project the love that it and its users deserve.
In the last two weeks I've also moved into a new place and I'm still getting settled in and sorting everything out.
Having said that, I do plan on restarting active development on this project in the next couple weeks.
I'm not entirely sure of the way going forward though.
There's an awesome pull request - #56 - which fixes a bunch of issues and revamps the UI. However that pull request is for the 0.5.X branch, which is in Objective-C. I started work on a Swift version (in the master branch) prior to the pull request, however that port is not complete. There is now a pull request - #70 - which fixes some build issues in that branch.
To be honest, I probably fucked up re: the port and the branches. I'm open to feedback on "the path" going forward.
Thoughts?
The text was updated successfully, but these errors were encountered: