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

Revert "Port both webhelper and markdown to webkit2gtk" #749

Closed
wants to merge 1 commit into from

Conversation

frlan
Copy link
Member

@frlan frlan commented Jun 3, 2018

Reverts #677

@frlan
Copy link
Member Author

frlan commented Jun 3, 2018

Puh. I really hope I didn't mess it up again

@b4n
Copy link
Member

b4n commented Jun 3, 2018

I'm not 100% sure how easy it is to reapply commits that have been reverted, short of making sure they are different commits (as otherwise they are already merged), so it might be slightly annoying for that other Markdown PR.

Also, for WebHelper I don't really mind so long as the small problems gets fixed; I wanted to keep GTK2 support, but it's also true that in many recent distros (including next Debian) there is no more webkit1, so it's just not so useful anymore; and the delta between webkit1 and webki2 is bigger than a few GTK2/GTK3 conditionals.
The only thing is that IIRC there are a few real issues with the webkit2 version that got merged, and it'd be sad to release with them, if they really exist. I might (or might not, no promises) have time to review and fix the issues before next release, but if people find and fix them already even better. My point is that the only thing that I really don't fancy with the merge are the potential bugs in the new version, not so much the drop of GTK2 support.

But either way is good, and it'd be just fine for me to work on a new PR if this is reverted.

@codebrainz
Copy link
Member

codebrainz commented Jun 3, 2018

so it might be slightly annoying for that other Markdown PR

Not sure if I'm just confused, but wasn't it already merged? I honestly thought this revert had already happened as I checked the diff and #746 and #677 had no differences for Markdown, so I didn't think it would be meaningful to merge until this revert happened.

Also, for WebHelper I don't really mind so long as the small problems gets fixed; I wanted to keep GTK2 support

That was my issue with the Markdown changes, and it was somewhat fixed in follow ups, though I'd like to improve it to still support all of the versions of webkit it's known to work with (the versions/package names are confusing but I guess webkit-1.0, webkitgtk-3.0, and webkit2gtk-4.0).

it'd be just fine for me to work on a new PR if this is reverted

IMO this is the best way, #677 should have never been made to change two plugins at once (especially plugins with different maintainers), I think that's where a lot of the confusion/problems came from. Maybe in the future we should just decline PRs which affect multiple plugins unless it's related to translations or build system or other more global stuff like that.

@elextr
Copy link
Member

elextr commented Jun 3, 2018

Not sure if I'm just confused, but wasn't it already merged? I honestly thought this revert had already happened as I checked the diff and #746 and #677 had no differences for Markdown, so I didn't think it would be meaningful to merge until this revert happened.

@frlan AFAICT the merge of #746 happened, but not the this revert, see markdown history. This is still a pull request that is not merged. Not sure what is going to happen when this revert is applied, but as @codebrainz says, since #746 is a subset of #677, reverting #677 now may also revert #746.

@frlan
Copy link
Member Author

frlan commented Jun 4, 2018

Hmmm #677 should be reverted with fb82de4 and idff against 1a575ee shows only markdown changes

@codebrainz
Copy link
Member

@frlan that's what I thought, but then what is this PR about?

@frlan
Copy link
Member Author

frlan commented Jun 4, 2018

@codebrainz litter. Going to clean it up

@frlan frlan closed this Jun 4, 2018
@frlan
Copy link
Member Author

frlan commented Jun 4, 2018

The PR was created by accident

@elextr
Copy link
Member

elextr commented Jun 4, 2018

I think whats confusing is that fb82de4 doesn't show in the history of, for example, markdown/src/viewer.c.

@frlan frlan deleted the revert-677-webkit2gtk branch September 19, 2021 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants