Cefpython123 #669
Cefpython123 #669
Conversation
Fixes: cztomczak#546 Had to include harfbuzz manually as newer Pango change this. See: eiskaltdcpp/eiskaltdcpp#413 https://gitlab.gnome.org/GNOME/pango/-/issues/387 Also had to add `-Wno-deprecated-declarations` to get this to compile because of the following errors that didn't seem to be coming from this code directly: warning: ‘GTimeVal’ is deprecated: Use 'GDateTime' instead warning: ‘GTypeDebugFlags’ is deprecated
…k#484). These callbacks were never called previously. Rename --no-run-examples flag to --unittests in build scripts.
… code at frame.pyx
|
when this can merge and publish one package? users are waiting |
|
Thanks for the port to 123. I think that this repository is no longer supported. Or may be wright small build instruction. I try build your fork with I download and unpack to And got this: Outputif you have any suggestion I will be grateful |
Are you able to use newer windows 10 sdk? This thread also suggests newer win10 sdk https://www.magpcss.org/ceforum/viewtopic.php?f=6&t=19436 |
Thank you! This version helped me. With this lib compiled and works. |
There was a problem hiding this comment.
Thank you for your contribution @linesight. You can find my code comments attached to this review.
| pyBrowser.GetMainFrame().SendProcessMessage(cef_types.PID_RENDERER, | ||
| 0, "DoJavascriptBindings", [{ | ||
| mainFrame = pyBrowser.GetMainFrame() | ||
| if mainFrame.IsValid(): |
There was a problem hiding this comment.
Should we print a log message when frame is invalid?
There was a problem hiding this comment.
I don't have a strong preference log or not log when frame is invalid
|
Also @linesight , why does GitHub show me "This branch has conflicts that must be resolved" message? |
|
I think I have addressed most (if not all ) techinical feedback from @cztomczak , really appreciate you also double check at upstream cef |
| @@ -11,10 +11,10 @@ cdef dict g_pyFrames = {} | |||
| # it shouldn't be kept global anymore. | |||
| cdef list g_unreferenced_frames = [] # [str unique identifier, ..] | |||
|
|
|||
| cdef object GetUniqueFrameId(int browserId, object frameId): | |||
| return str(browserId) +"#"+ str(frameId) | |||
| cdef str GetUniqueFrameId(int browserId, py_string frameId): | |||
There was a problem hiding this comment.
I guess previously it used object because of py2 <> py3 string differences. Hmm now we use Py2 in cython and Py3 for apps and this works fine?
There was a problem hiding this comment.
frameId is now of py_string , to align with how current upstream cef123 is treating frame id as a string, see
chromiumembedded/cef@2f1e782
I don't have histocial knowledge why object type was used, but I guess it was due to old frame id being int64_t type, didn't have nice or convenient mapping in cython
as frameId is now of string type, it is natural to change to function GetUniqueFrameId return type to string as well. More specifc type is, in general, more preferred than general type object.
For "Py2 in cython" comment, I think you are referring to
"language_level": 2,
still being used in cython_setup.py
I noticed this line at the beginning of development of this pull request, I tried to get rid of this line at first, then I met warning
cythoning cefpython_py311.pyx to cefpython_py311.cpp
D:\workspace\cefpython\venv311\Lib\site-packages\Cython\Compiler\Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: D:\workspace\cefpython\build\build_cefpython\cefpython_py311.pyx
tree = Parsing.p_module(s, pxd, full_module_name)
I didn't mean to introduce extra warning, then I put this language_level line back. How cython deals with this language_level internally isn't my primrary concern. Suppose you use python3 to execute this build instruction and example code, things work fine
There was a problem hiding this comment.
language_level can be set to 2 or 3 or even 3str. Changing it would require extensive testing of all APIs to make sure nothing got broken.
language_level (2/3/3str), default=None
Globally set the Python language level to be used for module compilation. Default is None, indicating compatibility with Python 3 in Cython 3.x and with Python 2 in Cython 0.x. To enable Python 3 source code semantics, set this to 3 (or 3str) at the start of a module or pass the “-3” or “–3str” command line options to the compiler. For Python 2 semantics, use 2 and “-2” accordingly. Before Cython 3.1, the 3str option enabled Python 3 semantics but did not change the str type and unprefixed string literals to unicode when the compiled code runs in Python 2.x. In Cython 3.1, 3str is an alias for 3. Language level 2 ignores x: int type annotations due to the int/long ambiguity. Note that cimported files inherit this setting from the module being compiled, unless they explicitly set their own language level. Included source files always inherit this setting.
Ref:
There was a problem hiding this comment.
In requirements.txt Cython version is Cython == 0.29.36. That version supports only language level 2 (Python 2).
I just checked and see latest version is Cython 3.0.12 which supports language level 3 and 3str:
https://github.com/cython/cython/releases
There was a problem hiding this comment.
In Cython i recall much confusion with str/unicode types. When you define function returning str type it really is Python 2 byte string from what I recall. To return unicode string I think that;s the reason for object type - when project had to support both Python 2 and Python 3. I'm not sure now, but it was all confusing.
There was a problem hiding this comment.
More on Cython strings: https://cython.readthedocs.io/en/stable/src/tutorial/strings.html
Cython supports four Python string types: bytes, str, unicode and basestring...
A nice read 😊
There was a problem hiding this comment.
Changing cython language level might change how strings are handled, or other things I don't know. I dont recommend changing it without re-testing of all APIs. Our unit tests are limited.
There was a problem hiding this comment.
Thanks for the detailed info. For the line of code in question, do you prefer me changing back to
cdef object GetUniqueFrameId(int browserId, str frameId):
or we can live with
cdef str GetUniqueFrameId(int browserId, py_string frameId):
?
I don't have strong preference of either way, I just slightly prefer str over object
There was a problem hiding this comment.
It's a cdef, so internal function. It doesn't much matter here. What matters is what you expose to Python API - a byte string or unicode string - we should be consistent with the rest of the API.
| @@ -80,7 +80,7 @@ | |||
| ALLOW_PARTIAL = False | |||
|
|
|||
| # Python versions | |||
| SUPPORTED_PYTHON_VERSIONS = [(2, 7), (3, 4), (3, 5), (3, 6), (3, 7), (3, 8), (3, 9)] | |||
| SUPPORTED_PYTHON_VERSIONS = [(2, 7), (3, 4), (3, 5), (3, 6), (3, 7), (3, 8), (3, 9), (3, 10), (3, 11)] | |||
There was a problem hiding this comment.
We should drop support Python 2.7. Also old Python 3 version such as 3.4, 3.5, 3.6.... we don't need that. We should only support latest popular Py 3 versions.
There was a problem hiding this comment.
Just for information. This PR is about CEF upgrade. Support in regards to Python versions is a separate issue.
There was a problem hiding this comment.
Thanks for the info.
|
@linesight Thank you for your work on this PR. I added a few comments, we're almost good to merge. I can accept a separate PR for documentation changes. Are all supported examples working fine? |
… is using complex iframe which is not good with cefpython javascript binding
I have checked all example python scripts, including those snippets, they are good to run, with below limitation -
so I am not able to confidently say they are good. Given my intent is windows focus, gtk isn't a popular GUI framework at windows, I would like to defer gtk investigation until cefpython-on-linux starts |
|
That's okay. As I understand this PR is for Windows support. |
|
We should also updated Build Instructions: https://github.com/cztomczak/cefpython/blob/master/docs/Build-instructions.md |
|
This PR was reverted. Please send changes to |
|
@linesight Also please squash your commits into one big commit with a title like "Update CEF to version 123.0.7 with Chromium-123.0.6312.46 (Windows tested)". If you don't squash, I can squash it and merge, but then I don't know how you will be credited in the GitHub Contributors view (Insights tab). |
|
Also documentation should be in one PR or another PR, but let's not mix in both PRs. I can merge when I have an opportunity to test on a Windows machine. |
|
As there has been multiple dicussions and multiple git commits at my branch to address each feedback, it is way too much effort to squash my commit at my branch on my side. I want my branch to preserve my original git commit history. It is better you squash it during merge. Or you can configure the pull request setting at your depot to allow squash merging. |
Added cef 123 support for python 3.10 and 3.11 at windows
Some of the fix is coming from https://github.com/llcc01/cefpython
qt.py example added pyqt6 support
addressing #652