Charts icon indicating copy to clipboard operation
Charts copied to clipboard

Review / merge PR #4297

Open 4np opened this issue 3 years ago • 6 comments

Pull Request #4297 has been open for almost a year now, but I see zero activity from repo owners / maintainers.

Honest question: if this project is all but abandoned, perhaps it should be marked as so. Then hopefully somebody will take over and maintain an active fork. Or use a bot that will close / label stale issues, because 614 open issues (the oldest one dating back to Apr 14, 2015) sounds quite unmanageable to me.

It is not very motivating to go through the time and effort to contribute code to the public repository if nobody takes the time to review or respond. I'm sure everybody is busy (so am I), but a year...

Issues that could potentially be solved by merging #4297 are at least the following issues: #4432, #4494, #4411, #4375, #3754, #1066, #4552, #4540, #4500

4np avatar Jan 29 '21 07:01 4np

I agree it would be nice to have the the functionality #4297 provides, and I appreciate the frustration you are feeling.

There are three issues:

  1. There is supposed to be feature parity and, as much as possible, identical implementations between this project and MPAndroidCharts
  2. The owner has expressed concerns over many of the open PRs/issues over the years, but has been MIA for some time now. It puts @liuxuan30 and I in a difficult spot to make decisions.
  3. This is the repo everyone lands on. A fork would require support from the community and at least @liuxuan30 if not also @danielgindi. Just as it can be unmotivating at times to contribute to this repo, it is unmotivating to maintain a repo nobody uses.

I have been feeling the pressure for some time now to fork the project, and remove the need to maintain parity with MPAndroidCharts. I think there are many opportunities to provide a simpler and more flexible implementation of the framework in doing so.

I will fork the repo now, and start to try and document a roadmap. I can currently promise much more active maintenance, but that doesn't necessarily mean all feature PRs get merged right away. I will spend the next couple days making breaking API changes and version it as a 0.X.Y until I feel we are in a good spot to "re-introduce" Charts. We can leverage the existing drawing code for Charts for the time being.

jjatie avatar Feb 02 '21 16:02 jjatie

First of all, let me say that it is much appreciated that both you and @liuxuan30 spend so much time working on the charting framework (and obviously for @danielgindi for creating it in the first place) 👍

I do think that it is important to have feature parity with MPAndroidCharts but it does not mean the API or underlying architecture should be completely identical as well. As long as you can accomplish the same in both frameworks, and they allow for the flexibility to customize where needed (that's where my PR comes in ;)).

Quite a number of 'issues' seem to be related to questions about using the framework rather than being real bugs / feature requests. Personally I would close those with a message that questions about how to use the framework should be asked on Stackoverlow, similar to what a lot of other repositories are doing. That way you end up with things that need to be addressed. Combine that with the bot that will close or label stale issues and it should be more manageable.

Thank you for stepping up! If the fork you will be working on is the one that will be getting all the fixes and improvements, I'm sure the community will follow.

4np avatar Feb 04 '21 10:02 4np

Hi @4np, thank you for your feedback.

Indeed I totally understand what you have said, and like what @jjatie said, we are lack of man power to review PRs and new features on a regular basis. Things are just keep coming and going, I think @danielgindi is occupied by his career and still responsive and active if we need him.

I remember we reached an agreement that if there are great PRs about new features & enhancements, we'd love to merge even it's not in MPAndroidCharts, but only if the PR meet all the requirements: code style, clean code, high demand from the community, well designed and implemented, no (obvious) side impact, so we won't have a hard time while thinking whether we could merge it, or we have to modify it to make it satisfy. However so far, there is almost no PRs meet this standard.

Taking an example is the rounded corner support, I do see there is a great PR #3754, though it is well implemented in terms of rounded bar itself, it has side effects and other issues we need the author to address, but the author was not responding after we pingged several times. I tried to take it on, but realize it's too complex for me to understand and konw how to solve all the issues in a short time, so we have to hold it.

Some other PRs are just more like a customizations and not clearly implemented.

For bug fixes and other simple ones, we will always review and merge. Currently @jjatie is refacotring the framework by a lot, so I hope people can contribute based on the new code base, rather than we fix the conflicts as I don't think we have the time.

The real issues for both Charts and MPAndroidChart are that we cannot guarantee the time and roadmap, as we all have a job to do. We cannot force people to work on this, it's all vulenteered( though we do have some fundings since last year! and you could get some benefits if you work on open source, e.g on your resume, or a free license for a software) So anything we planned is no easily achieved by different reasons, and it's no one's mistake or duty.

So we are kind of loosely committed to make sure this library is still funcitonal with latest iOS and tools, make refatcors and if there are some great PRs or bugfix, we are happy to review. But we couldn't guarantee more.

I would very apprecaite @jjatie have been helping this framework by a lot. And people like @jjatie is what we are exactly looking for, devoted, knowledgeful, and keep pushing and making things better. If we have more people like @jjatie then we could push the framework further. But it's totally fine people come and go, and our current working model makes sure the library will not be impacted if people come and go.

I believe @danielgindi would give me and @jjatie the trust & freedom to decide, but we still have the responsiblity to make sure it's still compatible as much as possible with MPAndroidCharts. I think most of the common demands and interests are satisfied already, and if you find a feature or something you think it should get merged, make sure it meet all the standards and we will be happy put it high priority to merge.

Last but not least, I'm grateful for everyone's help and also hope you could understand our current situation. Do whatever you want with the charts if you will have fun.

liuxuan30 avatar Feb 19 '21 02:02 liuxuan30

@jjatie I don't think you should fork it and move work into the fork. It will also scatter your work and this repo.

I think we could start a conversation with @danielgindi and see what he thinks, and perhaps we could get a pass to get more freedom on this, but before this I want to make 100% sure it will not give you or anyone pressure or obligation or being occupied by this project. Because if so, I would not suggest, as this project should be fun and motivated to work on.

It's totally fine to stop working on this if we are busy - the current working model makes sure it will be functional for most of the users; it's just not getting better, but also will not get worse on the other hand if we did half of job.

My philosophy is we want to provide a platform for normal users to create charts easily and give flexibity to customize for advanced users to create new chart shape or features that meet their goal; I personally want to avoid making the library super fat, we couldn't satisfy everyone's needs. We only provide a tool for them to create their own stuff.

I think what we have done in the past year to merge 4.0 and make it more swift meet the philosophy right? People have a better platform and code style to work on.

The best scenario for everyone I suppose is: for normal users, they can grab the code and create charts in a short time, as well as on Android; for advanced users, they can develop more in their fork to have fun; for anyone find something is valuable for most users and want to share, we help they meet the standard so we could merge. the bottom line of Collaborator role is to make the code base keep working on latest Xcode; fix what's broken / review PRs.

the hard part is some people think it's valuable while we are kind of hesitating, what we do for such PRs. I would suggest keep them in the author's fork, if we feel it's complex or impact the stability of the code base.

We still want to have important features for every one like rounded bars, but it would need the community's help and it's not an easy job for everyone.

I would start thinking how to use the fundings we have to motivate and reward for great work - though it's not a big bonus, it could afford a nice meal. We could allocate some money for the authors to praise their hard work, and some money for the Collaborators.

@jjatie what you think? I could start a conversation involves Daniel, you, and other members (like we could create a Chart org)

But this will only makes it more serious which I'm kind of hesitating, because again, I don't want to pressure any of you; I only hope everyone could have fun and enjoy. The current working model is fine with me. We could also alllocate the money even we are keeping the curren working model - you and others deserve it as well

liuxuan30 avatar Feb 19 '21 02:02 liuxuan30

For now I am going to leave my work as a fork. Where I can contribute back to this repo I will (like I have with the updated testing). I am experimenting a lot as I have general ideas about the direction I think things should move, but am uncertain on the specifics. Currently making any change to key types in the framework causes massive breakage which slows things down. When I have things in a good state, I'd be happy to have a bigger discussion about the future of the framework.

I think ultimately the ideas I have on how to improve the framework should have a very similar API from the common use case perspective, but allow easier customization for advanced use cases. I think the motivation for the PR referenced in this issue is great, but I think we can do much better with a larger refactor. It leaves me wondering if it's worthwhile to merge it if we think we are going to expose the same functionality in a different way.

General thoughts I have:

  • Current implementation of Objective-C support prevents us from taking advantage of many Swift features to improve the implementation of the framework.
  • Current Java inspired architecture feels burdensome both on the implementation of the framework, and in working with advanced use cases.
  • I think moving Obj-C support into a separate library would allow a better pure Swift implementation enabling a) better ergonomics in advanced use cases, b) meaningfully less code to maintain. I'd guess around to 40-50% less in my experimentation.
  • We aren't taking advantage of Swift's powerful type system, or it's preference for composition over inheritance. Given UIKit is an Obj-C framework we can only take it so far, but there's still a lot we can do.

jjatie avatar Feb 19 '21 14:02 jjatie

@liuxuan30 Thanks for your thoughts and feedback, I realize most people are contributing to this library besides their day-jobs in off-hours, so am I. I didn't have to create PR #4297 to contribute code back to the repository, it cost me quite some time I could have spent doing something else as well, but I think it will benefit many if the PR got merged. I appreciate all the time and effort that went into this project 👍 I just hope my PR makes it in at some point ;)

4np avatar Feb 22 '21 16:02 4np