Expose OnDragEnter & OnFileDialog in ClientHandler (#188, #361) #375

demonxweb · Jul 21, 2017

Expose DragHandler.OnDragEnter
Expose DialogHandler.onFileDialog

+ Expose DialogHandler.onFileDialog + Expose methods IsFile, IsFileName, IsFileNames in DragData. + Expose FileDialogCallback

cztomczak · Jul 21, 2017

Related issues:
#188 ("Implement CefDragHandler to allow dragging of local files into window to be cancelled")
#361 ("Expose DialogHandler and OnFileDialog callback")

cztomczak

On what platforms have you tested these changes? (I guess Windows only)

Please provide an example with which I can test the new functionality. You can either add unit tests. Or you can attach a script in this PR through gist: https://gist.github.com/ .

I see you've modified cef_version_win.h and set to an older version of CEF (v57). Looks like you're using CEF Python v57 and in this case you should have made this PR to branch cefpython57 instead of master. Your change alters CEF version and also removes DragData.GetImageHotspot() function which you can't do.

cztomczak · Jul 21, 2017

src/client_handler/client_handler.h

    }

-#if defined(OS_LINUX)
+//#if defined(OS_LINUX)


Please remove the comment

cztomczak · Jul 21, 2017

src/client_handler/client_handler.h

        return this;
    }
-#endif
+//#endif


And this one too

cztomczak · Jul 21, 2017

src/client_handler/dialog_handler.cpp

-#if defined(OS_LINUX)
-    return dialog_handler_->OnFileDialog(browser,
+    bool result;
+    result = DialogHandlerr_OnFileDialog(browser,


Please fix typo in the function name "DialogHandlerr_OnFileDialog" - there are two "r"

cztomczak · Jul 21, 2017

src/client_handler/dialog_handler.cpp

+    if(result){
+        return result;
+    }else{
+


Remove the blank line

cztomczak · Jul 21, 2017

src/client_handler/dialog_handler.cpp

+        return result;
+    }else{
+
+        #if defined(OS_LINUX)


Preprocessor conditions like #if #else #endif should be without indent as in the original source file

cztomczak · Jul 21, 2017

src/handlers/drag_handler.pyx

+            returnValue = callback(browser=pyBrowser,
+                     dragData=drag_data,
+                     mask=mask)
+


Too many blank lines

cztomczak · Jul 21, 2017

src/handlers/drag_handler.pyx

+        callback = pyBrowser.GetClientCallback("OnDragEnter")
+        if callback:
+            returnValue = callback(browser=pyBrowser,
+                     dragData=drag_data,


Fix indent for func arguments

cztomczak · Jul 21, 2017

src/handlers/drag_handler.pyx

+        sys.excepthook(exc_type, exc_value, exc_trace)
+
+    return False
+


Too many blank lines

cztomczak · Jul 21, 2017

src/version/cef_version_win.h

+
+#define CEF_VERSION "3.2987.1601.gf035232"
 #define CEF_VERSION_MAJOR 3
-#define CEF_COMMIT_NUMBER 1604


You can't modify this file in this PR, this was explained by me in the other comment

cztomczak · Jul 21, 2017

src/version/cef_version_win.h

+//#define CHROME_VERSION_BUILD 3029
+//#define CHROME_VERSION_PATCH 81
+
+#define CEF_VERSION "3.2987.1601.gf035232"


Looks like you're using CEF Python v57, but you made this PR to branch master which uses CEF Python v58.

demonxweb · Jul 21, 2017

Thanks for reviewing my code. I'll fixed the coding style.

I only tested in Windows 7 platform.
I'll try to provide a example or test case for you.
By the way, I'm interesting how did you test all of platform and python version.
I'll be glad that you cloud share your experiences with me.

Sorry about changed CEF_VERSION in cef_version_win.h.
Cause I didn't successfully compile CEF v58 yet.
I'll try to resolve this problem.

cztomczak · Jul 21, 2017

When running the build.py / build_distrib.py tools unit tests and examples are run automatically. The build_distrib.py tool tests multiple python versions automatically, read top comments in sources.

You can build master using these instructions that use Spotify binaries:
https://github.com/cztomczak/cefpython/blob/master/docs/Build-instructions.md#build-using-cef-binaries-from-spotify-automated-builds

But first you have to satisfy requirements:
https://github.com/cztomczak/cefpython/blob/master/docs/Build-instructions.md#requirements

cztomczak · Jul 21, 2017

If having problems building master please ask on the Forum, as this is off-topic here.

cztomczak · Jul 21, 2017

Useful resources in regards to installing multiple python versions with pyenv or editing cefpython's code using PyCharm are labeled Knowledge Base in the tracker.

demonxweb · Jul 24, 2017

Thanks for your comments.
I'll study that you provide information.

…methods in DragData.

cztomczak · Jul 26, 2017

src/client_handler/dialog_handler.cpp

+        return result;
+    }else{
+#if defined(OS_LINUX)
+        return dialog_handler_->OnFileDialog(browser,


Wrong indent, should be only 4 spaces.

cztomczak · Jul 26, 2017

api/DragData.md


 Table of contents:
 * [Methods](#methods)
+  * [IsFile](#isfile)


Sections should be ordered alphabetically, then re-generate TOC.

cztomczak · Jul 26, 2017

src/client_handler/drag_handler.cpp

+#include "drag_handler.h"
+
+
+bool DragHandler::OnDragEnter(CefRefPtr<CefBrowser> browser,


It seems that API doc for OnDragEnter is missing.

cztomczak · Jul 26, 2017

src/drag_data.pyx

+        PyToCefString(text,cefText)
+        self.cef_drag_data.get().SetFragmentText(cefText)
+
+    cpdef py_void SetFragmentHtml(self, html ):


An unnecessary space before closing parentheses.

cztomczak · Jul 26, 2017

src/drag_data.pyx


+    cpdef py_void SetFragmentText(self, text):
+        cdef CefString cefText
+        PyToCefString(text,cefText)


Please separate arguments with a space after comma.

cztomczak · Jul 26, 2017

src/drag_data.pyx

+
+    cpdef py_void SetFragmentHtml(self, html ):
+        cdef CefString cefHtml
+        PyToCefString(html,cefHtml)


cztomczak · Jul 26, 2017

src/drag_data.pyx

+        cdef CefString cefPath
+        cdef CefString cefDisplayName
+        PyToCefString(path,cefPath)
+        PyToCefString(display_name,cefDisplayName)


Space please.

cztomczak · Jul 26, 2017

src/drag_data.pyx

+        cdef CefString cefDisplayName
+        PyToCefString(path,cefPath)
+        PyToCefString(display_name,cefDisplayName)
+        self.cef_drag_data.get().AddFile(cefPath,cefDisplayName)


cztomczak · Jul 26, 2017

src/extern/cef/cef_drag_data.pxd

+
 cdef extern from "include/cef_drag_data.h":
    cdef cppclass CefDragData:
+        cpp_bool IsFile()


If the methods were ordered alphabetically before then please keep an alphabetic order when adding new ones.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+    def __init__(self):
+        self.OnPaint_called = False
+
+    def OnConsoleMessage(self, message, **_):


This duplicates code from main_test.py. Can we create _common.py file in unittests/ and move the duplicate code there? The DisplayHandler from main_test.py should be moved there and be imported in main_test.py and osr_test.py.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        browser.DragSourceEndedAt(303, 23, cef.DRAG_OPERATION_COPY )
+        browser.DragSourceSystemDragEnded()
+
+


Too many blank lines.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+            self.OnConsoleMessage_True = True
+            subtest_message(message)
+
+    def OnDragEnter(self, browser, dragData, mask):


We should keep the naming convention for handlers as in main_test.py, so that there are DragHandler (with OnDragEnter and StartDragging) and LoadHandler (with OnLoadEnd) classes.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+
+"""
+
+g_datauri = "data:text/html;base64," + base64.b64encode(g_datauri_data.encode(


Please get rid of the duplicate code. Define "html_to_data_uri" function in _common.py. Code from tutorial.py:

html = html.encode("utf-8", "replace") b64 = base64.b64encode(html).decode("utf-8", "replace") ret = "data:text/html;base64,{data}".format(data=b64) return ret

Replace these code with a call to html_to_data_uri in both main_test.py and osr_test.py.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+
+g_subtests_ran = 0
+
+def subtest_message(message):


Please get rid of duplicate code in all tests by moving subtest_message to _common.py along with the g_subtests_ran variable.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+
+
+class OSRTest_IsolatedTest(unittest.TestCase):
+    def test_dragdrop(self):


See this comment in main_test.py:

def test_main(self): """Main entry point.""" # All this code must run inside one single test, otherwise strange # things happen.

When working on main_test.py I noticed that you can't initialize CEF in one test run some other tests and then shutdown CEF in last test. I couldn't make it work with python's unittest module, strange things happened. The solution that worked was to run all tests inside on single test (test_main method) and introduce "subtest_message" function to report about results of various tests.

You can however run to separate tests by using IsolatedTest feature. See isolated_test.py - there are two classes defined IsolatedTest1 and IsolatedTest2 which run using separate python processes. This is the only way for CEF to work, because CEF can be initialized/shutdown only once during app lifetime.

I think the easiest thing is to define test_osr() and put both test_dragdrop() and test_dragdata() inside there. I don't mean to put all the code from test_dragdata() inside test_osr(), just make a call from there and the code should be in some separate function as it is now, but just don't make it a separate test.

The test_dragdata code seems to work now, however this seems to be invalid. Most of CEF API is intended to be called only after CEF was initialized. In your case CEF is initialized in test_dragdrop(), however this is a separate test and it shuts down CEF at the end, so after shut down you are not allowed to call any of CEF API anymore. You shouldn't instantiate cef.DragData() aftert CEF was shut down.

Just info for the future - with IsolatedTest you can have multiple separate tests in one test file and each of these isolated tests can initalize CEF and shutdown CEF, because they are run using separate processes. But I don't think we need to put these two tests (dragdata and dragdrop) as isolated tests, it would just increase the time to run both tests (CEF initalization, shutdown, message loop - it all takes time). If there is a case for it, when two tests need to be run in separate processes as they might affect each other, then sure go for it and create two isolated tests or more in one test file.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+
+    def test_dragdata(self):
+        dragData = cef.DragData()
+        testString = 'Testing DragData'


Please use double quotes everywhere to be consistent.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        testString = 'Testing DragData'
+        fileUri = r"C:\temp\ninja\README"
+        dragData.SetFragmentText(testString)
+        self.assertEqual(testString,dragData.GetFragmentText(),'SetFragmentText')


Sepearate arguments with spaces.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        subtest_message('DragData.GetFileNames() OK')
+
+if __name__ == "__main__":
+    unittests._test_runner.main(basename(__file__))


Missing blank line at eof.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        dragData = cef.DragData()
+        dragData.SetFragmentText('Test')
+        dragData.ResetFileContents()
+        browser.DragTargetDragEnter(dragData, 301, 21, cef.DRAG_OPERATION_COPY )


Remove unnecessary space before closing parantheses - on this line and other lines below it as well.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+
+    def OnLoadEnd(self,browser,frame,http_code):
+        subtest_message('cef.OnLoadEnd() OK')
+        #testing mouse event


Start a comment with a space and from upper case letter.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        browser.SendMouseMoveEvent(305, 25, False, cef.EVENTFLAG_LEFT_MOUSE_BUTTON)
+        browser.SendMouseClickEvent(305, 25, cef.MOUSEBUTTON_LEFT, True, 1, cef.EVENTFLAG_LEFT_MOUSE_BUTTON)
+
+        # testing drag event


Start a comment with a space and from upper case letter.

cztomczak · Jul 26, 2017

unittests/osr_test.py

+        subtest_message('cef.OnLoadEnd() OK')
+        #testing mouse event
+        browser.SendMouseClickEvent(300, 20, cef.MOUSEBUTTON_LEFT, False, 1, cef.EVENTFLAG_LEFT_MOUSE_BUTTON)
+        browser.SendMouseMoveEvent(305, 25, False, cef.EVENTFLAG_LEFT_MOUSE_BUTTON)


Add the code below before send the first mouse click event. As I explained on the Forum, you must first send move event to the location at which you will trigger a click event. Try this:

browser.SendMouseMoveEvent(305, 25, False, 0) browser.SendMouseClickEvent(300, 20, cef.MOUSEBUTTON_LEFT, False, 1, cef.EVENTFLAG_LEFT_MOUSE_BUTTON) browser.SendMouseMoveEvent(305, 25, False, cef.EVENTFLAG_LEFT_MOUSE_BUTTON) browser.SendMouseClickEvent(305, 25, cef.MOUSEBUTTON_LEFT, True, 1, cef.EVENTFLAG_LEFT_MOUSE_BUTTON)

If this code doesn't work as expected then add comment about it that StartDragging is currently never called and is on TODO list. Add a comment with a link to the Forum post in which I referenced code from upstream CEF unit tests on how it should be done.

It should be 300/20 coordinates:

browser.SendMouseMoveEvent(300, 20, False, 0)

cztomczak · Jul 26, 2017

Just FYI: I've just created Contributing code document.

cztomczak · Jul 27, 2017

api/DragHandler.md

+| mask | int |
+| __Return__ | bool |
+
+ Called when an external drag event enters the browser window.


This description is not enough. Please copy complete description from CEF headers using template like: "Description from upstream CEF: ...." - similarly to how it is done in API docs for some of other callbacks.

cztomczak · Jul 27, 2017

src/client_handler/dialog_handler.cpp

                                 CefRefPtr<CefFileDialogCallback> callback)
 {
+    bool result;
+    result = DialogHandler_OnFileDialog(browser,


You are missing API docs for OnFileDialog.

cztomczak · Jul 31, 2017

Hi DemonXWeb, I see that you still haven't addressed many of my comments. Also I've noted some other issues that I commented about a few times, but still aren't addressed correctly. If you don't have time to work on this further or are overwhelmed by the requirements I can understand this. The features are good and I want them merged. I can make the necessary changes myself later, but the schedule for that is unknown for now. Cheers!

cztomczak · Aug 18, 2018

Off-screen rendering test added in rev e72609c.

demon added 2 commits July 20, 2017 18:17

+ Expose DragHandler.OnDragEnter

454abe3

+ Expose DialogHandler.onFileDialog + Expose methods IsFile, IsFileName, IsFileNames in DragData. + Expose FileDialogCallback

* restructure dialog_handler to a correct file

c0ad480

cztomczak changed the title ~~Expose OnDragEnter & OnFileDialog in ClientHandler~~ Expose OnDragEnter & OnFileDialog in ClientHandler (#188, #361) Jul 21, 2017

cztomczak requested changes Jul 21, 2017

View reviewed changes

demon added 6 commits July 21, 2017 15:25

* Update docs for DragData

1300969

Update README

f18e366

* Fixed coding style.

1ee0a85

Fixed coding style.

aa346ab

Fixed coding style

99ed493

Fixed coding style.

f03034b

demon added 5 commits July 24, 2017 16:16

Fixed CEF_VERSION to 3.3029.1604.g364cd86

9f75d73

Expose SetFragmentText, SetFragmentHtml, AddFile & ResetFileContents …

2e8c0a8

…methods in DragData.

Add testing for file dialog & drag event.

7e5891f

Update docs for DragData

94c69b8

Update the doc of DragData

b1a9afd

cztomczak reviewed Jul 26, 2017

View reviewed changes

src/drag_data.pyx Outdated

cpdef py_void SetFragmentHtml(self, html ):

cdef CefString cefHtml

PyToCefString(html,cefHtml)

Copy link

Owner

cztomczak Jul 26, 2017

Choose a reason for hiding this comment

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

Here too.

cztomczak reviewed Jul 26, 2017

View reviewed changes

Fixed code for pull request.

db79ee0

Add unittests for OnFileDialog & OnDragDrop.

4f82ddb

cztomczak reviewed Jul 27, 2017

View reviewed changes

cztomczak mentioned this pull request Jul 27, 2017

Expose DialogHandler and OnFileDialog callback #361

Open

demon added 2 commits July 27, 2017 17:52

+

960de83

Update API docs.

3341550

cztomczak mentioned this pull request Aug 19, 2017

Unit tests support. Partially done. #59

Open

cztomczak force-pushed the master branch 4 times, most recently from abc5014 to 3ca4678 Compare February 8, 2019 17:44

cztomczak mentioned this pull request Jan 16, 2020

Implement CefDragHandler to allow dragging of local files into window to be cancelled #188

Open

cztomczak closed this Jul 23, 2020

		#include "drag_handler.h"


		bool DragHandler::OnDragEnter(CefRefPtr<CefBrowser> browser,

		browser.DragSourceEndedAt(303, 23, cef.DRAG_OPERATION_COPY )
		browser.DragSourceSystemDragEnded()


		"""

		g_datauri = "data:text/html;base64," + base64.b64encode(g_datauri_data.encode(



		class OSRTest_IsolatedTest(unittest.TestCase):
		def test_dragdrop(self):

Search code, repositories, users, issues, pull requests...

Uh oh!

Expose OnDragEnter & OnFileDialog in ClientHandler (#188, #361) #375

Expose OnDragEnter & OnFileDialog in ClientHandler (#188, #361) #375

Uh oh!

Conversation

demonxweb commented Jul 21, 2017

Uh oh!

cztomczak commented Jul 21, 2017

Uh oh!

cztomczak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demonxweb commented Jul 21, 2017

Uh oh!

cztomczak commented Jul 21, 2017

Uh oh!

cztomczak commented Jul 21, 2017

Uh oh!

cztomczak commented Jul 21, 2017

Uh oh!

demonxweb commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cztomczak left a comment •

edited

Loading