search

ViennaRSS / vienna-rss

Vienna is a free and open-source RSS/Atom newsreader for macOS.
https://www.vienna-rss.com
Apache License 2.0
1.85k stars 227 forks source link

[meta] Dropping 10.8 support #788

Closed josh64x2 closed 7 years ago

josh64x2 commented 7 years ago

@barijaona @Eitot I thought it would be best for us to discuss in a stand-alone issue since we don't have much luck finding each other on IRC/Matrix/Slack.

As a trial I have created two new branches from master on my fork:

  1. legacy3.1.x which I imagine will serve the same purpose as legacy3.0.x but for 10.8 users.
  2. modernisation which I envisage will replace master.

I'll try and cherry-pick commits in a sensible order from autolayout into modernisation and submit a PR to master so it can all be reviewed.

Does this sound acceptable?

josh64x2 commented 7 years ago

Do we want to squash all of the autolayout commits (the actual localisation and xib conversion ones) into one big commit?

Eitot commented 7 years ago

Sounds reasonable to me. Some of the XIB commits are massive though.

barijaona commented 7 years ago

The situation seems a bit difficult to handle, especially regarding Vienna.xcodeproj/project.pbxproj. My priority would be keeping internationalization and help books consistent between the two branches.

How about trying this for the reference branch ?

  1. checkout from situation at commit 736313b4e7887f177d2c71ed8251247841d337bb,
  2. reperform manually other localization fixes (I can’t guess if this would be easy without breaking too much thing),
  3. same thing for "Update project configuration and suppress ASIHTTPRequest issues"
  4. do the help book conversion (this could probably be cherry picked) and remove carbon dependency,

We could fork from there. One big commit for all conversion of xibs to auto layout seems to be an acceptable compromise.

josh64x2 commented 7 years ago

OK, my concern is the localisation fixes are dependent on base internationalisation, which is dependent on auto-layout... which I can't fix on 10.8.

josh64x2 commented 7 years ago

Helpbook bundle also relies on base internationalisation so that won't work on 10.8 either.

barijaona commented 7 years ago

This is why I'd give a try doing manually the other localization fixes, and guess what a massive diff would do from there.

josh64x2 commented 7 years ago

That brings me back to: It's a lot of work for very little gain. We have very few users on 10.8 that would benefit from localisation fixes.

I'll leave that as a task for you to do if you wish. In the meantime I'll fork from autolayout branch and make changes that will benefit > 99% of the users.

barijaona commented 7 years ago

Rethinking about it and trying to reformulate it, I don't care about 10.8 anymore than you. What really worries me for the future is the absence of an intelligible history. I hate having to deal with unclear history like those we had around June 2011-August 2011. I think situations like this hinders future debugging and development and they significantly discourage people from contributing to the project.

As an illustration, at commit 18732c7c2da24b884944bba8509ebcb76616bf9c, the french localization is OK. But it is completely broken at the next commit 23f20be427532263eee4eb92b81f7fb219acabe8. Why ? It seems unclear for me.

Then french localization seems to be restored at commit 2a9b733cebf80b81d948653d8520233100468b85. Did we break something in between ? I hope no, but I feel insecure and I'd like these massive changes to be better documented ; otherwise even I would be afraid that I could break things by submitting a contribution.

josh64x2 commented 7 years ago

OK I will look into June 2011-August 2011 and try to understand what you mean. Thank you for taking the time to explain! I know this is difficult to communicate over text + slight language barrier.

Eitot commented 7 years ago

I understand the concerns, but I would just like to point out that I could not discriminately work file-by-file while maintaining the integrity of the pbxproj.file. This file is such a pain to manage that I chose to do everything in batches rather than in completely linear steps. That is why something breaks from one commit to another. My thought was that these larges IB changes are better reviewed by looking at the end result, rather than the process in too much detail.

barijaona commented 7 years ago

I suggest one of you guys add comments to commits (especially at merges). I suggest using something like GitUp which I find particularly convenient for this. It could also allow you to perform minor reorganization.

Then you would resubmit the commented out branch to be merged into master.

barijaona commented 7 years ago

A proposal update to the localization guide would also be useful.

josh64x2 commented 7 years ago

@barijaona are you asking for the commits to be amended with different comments?

barijaona commented 7 years ago

I am currently beginning a deeper review of your code.

My current impression : you gave some info about what you did. But given the complex interrelations between different subjects (localization fixes, base internationalization, autolayout) and the important number of commits and modified objects, it would be nice to have more informations about why you did some things. I can find some comments about this in the recent GitHub issues, but they will soon be buried in time and I prefer to add them to the text of your merge commits.

But have a nice week-end! I might ask questions from time to time and I will probably get back to you on Monday.

josh64x2 commented 7 years ago

@barijaona I apologise that I forgot about the 7 day review window before PRs are accepted (according to the contribution guide) - I personally think this is too long and 3 days is probably better but that's my own opinion.

However, I think we need to have a level of trust between us. If one of us thinks that something is acceptable to be merged then that should be OK. E.g. PR #725 and #734.

@Eitot I am sorry for the confusion and uncertainty around the autolayout work. I hope it does not discourage you from continuing your valuable contributions!

Keep in mind it is generally OK for individual commits to break things (such as French localisation that you mentioned), as long as it is fixed BEFORE merging into a main branch.

With regards to the localisation/translation guide updates - I had already raised #763 for this.

barijaona commented 7 years ago

Please, let me put things straight : I think nobody should ever feel guilty because of a contribution he made or a decision he took. It is particularly true in the current situation, because nothing has been merged yet into ViennaRSS's master. ViennaRSS's master should remain the one and only sacred cow in this project 😄 !

What alarmed me first was PR #786. Though it was submitted to Vienna's autolayout branch, I considered it broke one important rule, which is inherited from Agile programming : in order to be able to deliver functional prototypes as often as possible, it is really important to keep things which are not directly related into separate branches. This allows quicker and safer merges. This is why I asked the refactoring of app controller to be submitted separately because it was clearly unrelated to the migration of XIB's to autolayout.

What alarmed me afterwards was the announcement of the intention to make autolayout become the new master. This led me to submit autolayout to a deeper scrutiny (remember the stuff about sacred cow ? 🤣 ), and I noticed that this branch broke the previous rule at various points.

This is why I intent to refactor it. I hope to have by the end of the week-end at least 3 separate branches which should be tested and reviewed separately :

In an ideal world, I would separate autolayout per se and changes to localization. But I realize that the initial situation was so messy that it is probably impossible to achieve this. At least, I intent to better document what has been done and why.

I hope my code review does not discourage any of you. But I think that Vienna 2's codebase survival to almost 10 years of history is a great experience, and I consider it is now important that we stick to a minimum of discipline for large changes.

barijaona commented 7 years ago

Regarding the 7 days window review. Yes, it is a bit long, but :

barijaona commented 7 years ago

And the 7 days window is a maximum which only applies to your own work (you can merge yourself if nobody objected in the time period). On the contrary, as soon as another member of the team has performed an independent review of your code and is satisfied with it, he may merge it immediately.

barijaona commented 7 years ago

(Edited)

Hi all ! Could you have a look at these 3 branches in my repo ?

modernization2 is a mockup of what could be the future master. modernization2 is identical to current Eitot/autolayout (apart from a fix to image references in French help files). But from my point of view, its history is easier to understand for a third party.

My suggested approach :

barijaona commented 7 years ago

Sorry, refer to branch modernization2 instead of modernization in the above message. Everything else unchanged.

josh64x2 commented 7 years ago

OK I should have a chance to review tomorrow.

josh64x2 commented 7 years ago

@barijaona I have had a chance to review. projectevolution looks good! I find the history of AutoLayout+BaseInternationalization no easier to read than the history of https://github.com/josh64x2/vienna-rss/commits/master-test which is autolayout merged into master (but called master-test).

modernization2 has shared commits with AutoLayout+BaseInternationalization so it isn't a unique set of changes.

Maybe you could try your suggested approach by merging into a new branch in your fork called master-test and then I can take a look at the result? It might help me understand better 😄

I see in a previous comment that you were concerned that the changes that had been merged into autolayout were not related to autolayout. I can rename the autolayout branch to develop or future-master or something if that makes it easier to see not all changes were related to autolayout functionality. In the commit log you can see that separate features branches did exist for each distinct feature (as per the Agile approach you mentioned). I'm not sure if that would address some of your concerns.

If we were to do that, we could create legacy-3.1.x and pull your projectevolution changes into it. e.g. https://github.com/josh64x2/vienna-rss/commits/legacy3.1.x

Anyway I'll take a look at your master-test when it is ready!

barijaona commented 7 years ago

OK, I think I fixed it. Could you review my master-test branch at https://github.com/barijaona/vienna-rss/tree/master-test ?

My version of legacy3.1.x goes a little further than yours, as it incorporates the new organization for help files : https://github.com/barijaona/vienna-rss/commits/legacy3.1.x

barijaona commented 7 years ago

There is room for small enhancement (Vienna.help should not appear in Xcode) but I'm very happy with the current result. Thanks to your combined work, UnifiedDisplayView's behavior is much better.

But it is very important to incorporate the solution to issue #724 that @Josh64x2 apparently found. Could you publish its current state and I will try to incorporate it in one of my branch ?

josh64x2 commented 7 years ago

I don't see a problem with Vienna.help being in Xcode - it is a dependency of the app and it is nice to see all resources for the app inside Xcode. This is actually the way Apple and all other tutorials do their help books.

I can only incorporate half of the solution, and that is enabling layer backed views on our xibs. The network half will require dropping 10.8 so I can use NSURLSession API instead of needing to use both the deprecated NSURLConnection AND NSURLSession

barijaona commented 7 years ago

I don't see a problem with Vienna.help being in Xcode - it is a dependency of the app and it is nice to see all resources for the app inside Xcode. This is actually the way Apple and all other tutorials do their help books.

Apparently, I didn't pick the good version, and it should probably in a product group.

I can only incorporate half of the solution, and that is enabling layer backed views on our xibs. The network half will require dropping 10.8 so I can use NSURLSession API instead of needing to use both the deprecated NSURLConnection AND NSURLSession

The crashlogs I have encountered so far make me suspect the xibs more than network stacks. So if you publish your solution, I can try cherry picking so that we can start 3.2 development from good foundations.

josh64x2 commented 7 years ago

Yes, the first crash logs show graphics issues. After layer backed views are in enabled, the logs show problems with waking from sleep and the networking calls being synchronous and hanging.

My layer-backed view changes were made when everything was in .nibs and so the git commits wont be able to be applied to the current branch.

barijaona commented 7 years ago

I understand this, but could you publish anyway your latest layer-backed branch so that everybody may have a better understanding of the future merges to come into master.

Your layer-backed changes should probably be applied to master before autolayout. If you publish it, I will propose a new version of master-test.

josh64x2 commented 7 years ago

If I make changes to nibs before we apply autolayout, the changes are going to get wiped out as soon as we merge autolayout because the nibs will no longer exist and enabling layer backed views is done through interface builder. You can see that here https://github.com/josh64x2/vienna-rss/commit/1337d2ae3406af73f401a78ba217bf3f4b17c84f (it turns out I had pushed an old version of the branch to my fork).

The only user that is experiencing issues is not using 10.8 and so these changes do not need to be applied to 10.8 so the benefit would be zero.

You need to understand we have LOTS of users and we need to prioritise their needs over our disagreements. I have not been able to do any meaningful work on Vienna for over a week as we have not been able to come to agreement on a merge strategy for the numerous changes @Eitot contributed.

I am frustrated and it is making me not want to work on Vienna. I realise I am free to fork the project and work on it myself, but I would like for us to be able to find a solution to move forward.

josh64x2 commented 7 years ago

Can you please submit the pull requests for your branches in the order you want me to merge them into master? Thanks!

Eitot commented 7 years ago

I think we can drop this commit, no? https://github.com/barijaona/vienna-rss/commit/439e4ef76c9167d5272b6c8d252d8afff3b5a0b7

josh64x2 commented 7 years ago

Yeah I guess we can because it's only required for 10.8 which won't be getting the autolayout-UI specific changes

barijaona commented 7 years ago

Josh wrote :

You need to understand we have LOTS of users and we need to prioritise their needs over our disagreements. I have not been able to do any meaningful work on Vienna for over a week as we have not been able to come to agreement on a merge strategy for the numerous changes @Eitot contributed. I am frustrated and it is making me not want to work on Vienna. I realise I am free to fork the project and work on it myself, but I would like for us to be able to find a solution to move forward.

I understand your impatience, but I am pretty sure here that preserving the long term future was a well spent time.

You guys did an excellent work coding in the recent weeks. But in your impatience to clean old code, you probably didn't give much thought to a strategy regarding merging and syncing code. Merging is THE number one issue you have to deal with when working with several people on a SCM. Everyone can do what (s)he likes in its private branches, but for submitting anything to be merged into a significant size project (even it is not Linux), having bite size pieces is really important.

Even when I was the single active contributor to Vienna, I never considered I had any privilege regarding this and strictly stuck to the above mentioned discipline. Especially :

If I had the slightest idea that you created the Vienna/autolayout branch with the idea that it could become a new master, I would have strongly warned you not to do that ; because I saw too many good projects (both closed and open source) literally die from heterogenous code management practices.

As a compromise, I did some refactoring to your works (I was probably more frustrated than you 😸) and I will begin submitting in a few minutes, but please, please, never submit any spaghetti plate again to Vienna's main repo ! :smile:

Final (quote from Chris Beam) :

a well-cared for log is a beautiful and useful thing. git blame, revert, rebase, log, shortlog and other subcommands come to life. Reviewing others’ commits and pull requests becomes something worth doing, and suddenly can be done independently. Understanding why something happened months or years ago becomes not only possible but efficient. A project’s long-term success rests (among other things) on its maintainability, and a maintainer has few tools more powerful than his project’s log. It’s worth taking the time to learn how to care for one properly. What may be a hassle at first soon becomes habit, and eventually a source of pride and productivity for all involved.

barijaona commented 7 years ago

@Eitot wrote :

I think we can drop this commit, no? barijaona@439e4ef

From a strict technical perspective, probably yes. But I prefer keeping the same structure for all localizations.

barijaona commented 7 years ago

For your information, the future history would have a lot of similarity with this (updated branch master-test2).

josh64x2 commented 7 years ago

@barijaona I still see you are not making any real compromise - even with https://github.com/barijaona/vienna-rss/commit/439e4ef it is all about what you want to do.

Please consider the views of the other two main contributors.

josh64x2 commented 7 years ago

I think I have deviated a little bit from the original purpose of this issue so I will close it. I have accepted the PR and look forward to making more substantial changes once we branch legacy-3.1.x.

Thank you for taking the time to pull the commits out and put them back in separate branches.

barijaona commented 7 years ago

Josh, I have to express my surprise and disappointment on your reaction. I am afraid you are taking what should remain a technical review as a personal affair.

This is the first time ever I had a real problem with a pull request and I think I took enough time to explain my reasons. When I understood that you were not in a position to change things quickly, I spent nights to design what I consider a better solution.

Did I break your contributions or @Eitot's ones ? Quite the contrary. If you take the time to compare situation of my master-test2 branch at commit 9f54c9f46b765d049b78b4669ae3f886bf6eac05 with situation of @Eitot's autolayout branch at commit e35b1a057304e754210a9b52711a4824ae20d830, you will notice that I took a lot of precautions to get exactly the same final result (with the exception of some localized .html help files, which I discovered where lost during the migration process and that I fixed).

Similar things can be said regarding commit 439e4ef . I did not invent it, you wrote it, Eitot cherry-picked it, and I just followed his track. I just expressed the opinion that I saw no inconvenience in keeping it, but you are considering that I want to stubbornly stick to my (?) way. If you'd voice a real technical advantage in removing it, I'd be more than happy to do so. But this will require additional testings, while I thought you wanted to go forward quickly :smile:

Next, regarding team relationship : I sincerely apologize if something I did hurt you, but I honestly think I largely respect the laisser faire attitude which is at the core of the open source spirit. For instance, I let you try the ViennaNG route, even if I was a bit skeptical of the cost/benefit ratio and did not really see what kind of radical difference you wanted to introduce. I didn't even express any concern when you announced on Vienna's blog that it would be available "soon", even if I thought that such an announcement was a bit early. Another exemple : you tried to maintain 10.8 compatibility with classical Vienna, I did not discuss it. You later came to the conclusion it would be better to drop 10.8, I encouraged you to do so.

But laisser faire attitude does not exclude code review. As an example, you asked me to change some of my coding habits, I complied and got used to it later. Why should my observations regarding code merging strategy be treated completely differently ?

I hope my words do not look too harsh. Remember that English is not my native language and different countries mean different cultures.

Eitot commented 7 years ago

I think we can drop this commit, no? barijaona@439e4ef

From a strict technical perspective, probably yes. But I prefer keeping the same structure for all localizations.

But now we have additional maintenance for no practical reason. There aren’t just strings files in there, but also plist files. It is also possible that the en.strings might diverge from the base in the future, if we are not paying attention. After all, localization of XIB files is different from localizable.strings, because there is no pattern matching involved.

I still have to update the strings files with Xcode’s export/import feature, so that any unused strings are discarded and the comments are updated.

josh64x2 commented 7 years ago

@barijaona the technical reason for not keeping https://github.com/ViennaRSS/vienna-rss/commit/439e4ef76c9167d5272b6c8d252d8afff3b5a0b7 was already explained:

  1. It is only needed for autolayout on 10.8 which we are not proceeding with
  2. Keeping it increases bloat and maintenance

I will email you about the other stuff.

barijaona commented 7 years ago

We'd need to keep at least en.lproj/Localizable.strings. There are entries where the key in source code is different from the english text. Ex : line 62, 90, 91…

For the other .strings, I see it mainly as a question of personal preferences. For instance, there was recently a questioning about the use of the term "folder". If we decide to replace "folder" with "feed", one would either have to :

I might seem conservative (probably because I am without knowing it 😃) , but I learned the hard way that if you see strange habits or customs without noticing that they have significant bad effects, it's less hassle to not attempt to change them…

josh64x2 commented 7 years ago

Hi @barijaona I think you are confused - Base.lproj IS essentially the same as en.lproj because in xcode we set the base localisation to be English.

In your example, we would simply change "folder" to "feed" in the file Interfaces/Base.lproj/Localizable.strings for the English language. If we had selected French as the base language, we would not need fr.lproj because Interfaces/Base.lproj/Localizable.strings would contain french strings. Does this make sense?

That is why en.lproj is not needed - it is a duplicate of base.lproj in our configuration. There was an implementation "issue" in 10.8 where you needed en.lproj in addition to Base.lproj but that doesn't affect > 10.8

edit: see https://github.com/ViennaRSS/vienna-rss/blob/autolayout/Interfaces/Base.lproj/Localizable.strings which should make it more clear.

Eitot commented 7 years ago

Just to add to what Josh has said, changing a string in a XIB file has no direct consequences for any .strings file. As I said above, there is no pattern matching for .strings files that belong to XIB files.

In Localizable.strings, you would have this:

"Cancel" = "Annuler";

In SearchFolder.strings, you have this:

"94.title" = "Nom du dossier intelligent :";

The “94.title” is a unique object identifier in the XIB file. We can change the string in the XIB file at any time, without having to change any translations, because the identifiers won't change (unless the object is replaced of course).

barijaona commented 7 years ago

I understand, but cf. my specific remark regarding Localizable.strings which already contains some differences between keys and displayed texts.

.m source code refers to some wording here, but the texts shown to the user is different even in English.

I initially found it very confusing, but later saw it had advantages.

See it as an additional abstraction layer : developer (for .m or .swift source) or UI implementer (for .xib source) may use some wording, but UI designer can choose another wording to be presented to the end user.

Consider a situation where some programmers are used to American English, others to British English, others to Indian English and still others (like myself 😺 ) to some kind of Pidgin. No need to think a lot about the adequate term to use in source, every programmer will use what (s)he fits her/him, and you will have en-us, en-uk, en-au and en localization.

See the paragraph named Pseudolocalization at the bottom of https://developer.apple.com/library/content/documentation/MacOSX/Conceptual/BPInternational/MaintaingYourOwnStringsFiles/MaintaingYourOwnStringsFiles.html . Some people might see as a good habit that programmers systematically use a Pseudolocalization.

barijaona commented 7 years ago

Did someone have feedback about the use of OHAutoNIBi18n ?

See the Comparison with Xcode 4.5's "Base Localization" part in the ReadMe. Note that I am completely unsure this is a better solution, but there is clearly room for improvement.

Eitot commented 7 years ago

You are still not making a case for keeping the en localisation files though. As Josh said, the Localizable.strings files in Base and en.lproj are identical. For all intents and purposes, the base Localizable.strings file is that abstraction layer you write of. I do not think that it makes things easier if yet another layer is added. Potentially, we can end up with having three different versions – source → Base → en – which are meant to refer to the same thing. Not to mention that translators may end up getting confused if they base their localisation on any of these particular versions.

I personally avoid abstraction if it means doing the same thing multiple times. That alone causes unnecessary complexity. Ideally, we should settle on a variant of English and not work around it. Moreover, eventually the Localizable.strings files should become less important as we are making more use of Interface Builder. The toolbar, for instance, can be generated entirely in Interface Builder.

barijaona commented 7 years ago

You're right, no requirement to keep en.lproj/Localization.strings.

But I still think it's just a question of personal preferences. I dislike Interface Builder (it was and still is a mighty Git history polluter…) and prefer inspecting English text files to using it.

Anyway, majority rules. I'll stop the discussion here and remove en.lproj.

Eitot commented 7 years ago

I agree that IB is not ideal for git and continuous integration. I also dislike that you never get a good feel for what is happening behind the scenes as you are working with objects. There is a certain beauty to instantiating components yourself and only setting the values that you need, rather than what IB all does.

However, I really, really do not like 'dumb' code that serves no other purpose than instantiating view objects and adding them to the view hierarchy. In that sense, having this sink is a relief.

barijaona commented 7 years ago

Updated branch master-test3 in my personal repo.

barijaona commented 7 years ago

If you don't mind, I'll squash a few commits to make the history easier to read.