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

Port WebKit 3.0 to WebKit2 4.0 API #3

Closed
wants to merge 5 commits into from

Conversation

chimosky
Copy link
Member

No description provided.

@chimosky
Copy link
Member Author

@quozl, kindly review.

@quozl
Copy link

quozl commented Jul 4, 2018

Thanks. Reviewed to fea2c3a.

  • change the name of the callback; as it was named consistently with the signal, and now isn't,
  • change the name and number of arguments of the callback to match the documentation; as the callback no longer executes properly,
  • remove your change to html/es/html; it is unrelated to the port,
  • test and finish porting; there are other changes you've yet to make, as the activity does work correctly; images are not loading, and the back button is not available after the first click; these problems are so severe you would have noticed them if you had tested, so it looks like you were not yet at the stage of testing,
  • use more detail in commit messages, see guide to contributing; in particular your removal of set_full_content_zoom is unfortunate, yet you didn't explain why you removed it rather than use an alternative.

Add set_zoom_level to webview
 * in the previous commit set_full_zoom_level was removed
   because it was deprecated and it has been replaced with
   set_zoom_level, set_full_zoom_level which was True scales
   the full content view but since it's been removed using
   set_zoom_level gives you the same effect
…d methods

Remove html/es/html as it wasn't there before
@chimosky
Copy link
Member Author

chimosky commented Jul 4, 2018

Changes made

  • change the name of the callback; as it was named consistently with the signal, and now isn't,
  • remove your change to html/es/html; it is unrelated to the port,
  • test and finish porting; there are other changes you've yet to make, as the activity does work correctly; images are not loading, and the back button is not available after the first click; these problems are so severe you would have noticed them if you had tested, so it looks like you were not yet at the stage of testing,

Images are loading properly for me, the back button isn't available on the first click it is on the second - click on something else first, could be home button; also occurs in v20 -

  • use more detail in commit messages, see guide to contributing; in particular your removal of set_full_content_zoom is unfortunate, yet you didn't explain why you removed it rather than use an alternative.

Noted.

@quozl
Copy link

quozl commented Jul 5, 2018

the back button isn't available on the first click it is on the second - click on something else first, could be home button; also occurs in v20 -

In my tests, it does not occur in v20 on Fedora 18, or v20 on Ubuntu 18.04.

  • Please fix the back button, thanks.

Reviewed to 3af8291.

  • You've removed the zoom in and zoom out buttons. Please put them back and make them work properly with the WebKit2 API. Thanks!

@chimosky
Copy link
Member Author

chimosky commented Jul 5, 2018

Just thought i should share, I'll see if i can implement them.

@quozl
Copy link

quozl commented Jul 5, 2018

Thanks, interesting. I imagine you will need to call set_zoom_level in the zoom in and out callbacks. In your 7c4d34d what's your purpose in calling set_zoom_level and passing the level that it already has set? Is there some needed side-effect?

@chimosky
Copy link
Member Author

chimosky commented Jul 6, 2018

As set_full_content_zoom was removed in fea2c3a, i added it to serve as a replacement.

@chimosky
Copy link
Member Author

chimosky commented Jul 6, 2018

@quozl the current zoom level seems to be working properly but requires an opposite reaction before it works again - you zoom in once then zoom out before you can zoom in again -, so basically the both buttons only work once not on multiple clicks.

@quozl
Copy link

quozl commented Jul 12, 2018

Thanks. Let me know when you have it working correctly; zoom buttons should work as they did before.

All zoom buttons now work properly
@chimosky
Copy link
Member Author

@quozl kindly review.

@quozl
Copy link

quozl commented Jul 26, 2018

Duplicate of godiard#99

@quozl quozl marked this as a duplicate of godiard/help-activity#99 Jul 26, 2018
@quozl quozl closed this Jul 26, 2018
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.

2 participants