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

Remove Facebook support #1958

Merged
merged 2 commits into from Feb 2, 2019
Merged

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Feb 1, 2019

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

facebook: remove the feature from the code base

Remove from:

  • unit tests
  • desktop widgets
  • preferences
  • core intergration
  • cmakefiles
  • build scripts
  • icons
  • docs

Also remove the plugins and social network integration.

Changes made:

see above.

Related issues:

none for the removal

Additional information:

  • i will also close all existing open facebook issues in the tracker (edit: done)

Release note:

added

Documentation change:

  • remove the facebook section from all the multi-language docs (TXT source),
  • remove the images too.

Mentions:

@dirkhh @willemferguson

@neolit123
Copy link
Member Author

neolit123 commented Feb 1, 2019

@willemferguson could you please check if i haven't broken the docs in any way?

you can pull this as a patch like so:

wget https://github.com/Subsurface-divelog/subsurface/pull/1958.diff
git apply 1958.diff

@dirkhh
Copy link
Collaborator

dirkhh commented Feb 1, 2019

Interesting that all non Windows/Android builds failed. And equally interesting, I guess, that those completed, given that type of errors that I see in the other builds.
@neolit123 are you still working on this, or do you want me to fix the build errors so this can be merged?

@neolit123
Copy link
Member Author

looks like the tests are failing at:

CMake Error at tests/CMakeLists.txt:44 (add_executable):

i will fix that and watch the build again.

@neolit123 neolit123 changed the title WIP: Remove Facebook support Remove Facebook support Feb 1, 2019
@neolit123
Copy link
Member Author

@dirkhh
i have removed the WIP.

all tests pass except the Xcode ones with:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

possibly a temporary issue.

@dirkhh
Copy link
Collaborator

dirkhh commented Feb 1, 2019

the iOS failure is that there's still a reference to the FB preferences in the qmake file:

packaging/ios/Subsurface-mobile.pro:    ../../core/settings/qPrefFacebook.cpp \
packaging/ios/Subsurface-mobile.pro:    ../../core/settings/qPrefFacebook.h \

And a quick git grep -i facebook shows a ton of hits in .pot files and translations (those are all fine), but it also shows me that we still refer to this in the FAQ (I'll fix that myself) and in a few other places in the code...

@dirkhh
Copy link
Collaborator

dirkhh commented Feb 1, 2019

I also think that core/isocialnetworkintegration.* and references to it should be removed while we do this, right?

@dirkhh
Copy link
Collaborator

dirkhh commented Feb 1, 2019

I'm pretty sure the same is true for core/pluginmanager.*

@neolit123
Copy link
Member Author

the iOS failure is that there's still a reference to the FB preferences in the qmake file:

ack.

I also think that core/isocialnetworkintegration.* and references to it should be removed while we do
this, right?

i left those and the plugin manager intentionally.
LMK if you want them removed.

@dirkhh
Copy link
Collaborator

dirkhh commented Feb 1, 2019

Since this all came in when the FB support was added, it seems the right time to also remove them.

@neolit123 neolit123 force-pushed the remove-facebook branch 2 times, most recently from 976411a to 568601c Compare February 1, 2019 19:17
Remove from:
- unit tests
- desktop widgets
- preferences
- core intergration
- cmakefiles
- build scripts
- icons
- docs

Also remove the plugins and social network integration.

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>
Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>
@neolit123
Copy link
Member Author

all tests have passed.

Copy link
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks for taking care of that, Lubomir.

@dirkhh dirkhh merged commit 3502afa into subsurface:master Feb 2, 2019
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.

None yet

2 participants