ubuntu-webapps-bugs team mailing list archive

[Chris Coulson <chris.coulson@xxxxxxxxxxxxx>]

Public bug reported:

I've tried to rearrange things to make it clear how code should be split
between, say, WebViewAdapter in qt/core/glue and WebView in
qt/core/browser, but the code split still isn't really all that clear
(even to me). In addition to that,  there's other problems:

- There's a lot of implementation details exposed between the 2 directories, using private methods and friend statements which all seems a bit wrong.
- Using the WebView case again, there may be a legitimate reason for another class in Oxide to do OxideQQuickWebViewPrivate::get() in order to access a subset of methods on OxideQQuickWebViewPrivate, but it also exposes the entire API surface of WebViewAdapter as its methods have to be public in order to be accessible from OxideQQuickWebView (unless we make them protected and use a friend statement).

I think we should refactor it all to do this instead (using webview as
an example):

(sorry if this is not clear)

class OxideQQuickWebView : public QQuickItem {
 public:
  ...
  <public API>
  ...

 private:
  QScopedPointer<OxideQQuickWebViewPrivate> d_ptr;
};

class OxideQQuickWebViewPrivate : public oxide::qt::WebViewProxyClient {
 public:
  static OxideQQuickWebViewPrivate* get(OxideQQuickWebView* q);
  ...
  <methods available to other code in qt/quick/api>
  ...

 private:
  ...
  <implementation of WebViewProxyClient>
  ...

  QScopedPointer<oxide::qt::WebViewProxy> proxy_;
};

WebViewProxy and WebViewProxyClient are abstract interfaces in
qt/core/glue, although WebViewProxy has a static create() function.

In qt/core/browser:

WebView : public WebViewProxy {
 public:
  ...
  <methods available to code in qt/core/browser>
  ...

 private:
  ...
  <implementation of WebViewProxy>
  ...

  WebViewProxyClient* client_;
};

This fixes the above problems (we don't have to expose implementation
details between qt/core/browser and qt/core/glue, the WebViewProxy
instance is private to OxideQQuickWebViewPrivate and its API isn't
exposed to classes that call OxideQQuickWebViewPrivate::get(), and
developers don't need to spend time trying to figure out which directory
to put code in to)

** Affects: oxide
     Importance: Low
         Status: Triaged

** Changed in: oxide
   Importance: Undecided => Low

** Changed in: oxide
       Status: New => Triaged

** Description changed:

- I've tried to rearrange code to make it clearer which code belongs in,
- say, WebViewAdapter in qt/core/glue and WebView in qt/core/browser, but
- the code split still isn't really all that clear (even to me). In
- addition to that,  there's other problems:
+ I've tried to rearrange things to make it clear how code should be split
+ between, say, WebViewAdapter in qt/core/glue and WebView in
+ qt/core/browser, but the code split still isn't really all that clear
+ (even to me). In addition to that,  there's other problems:
  
  - There's a lot of implementation details exposed between the 2 directories, using private methods and friend statements which all seems a bit wrong.
  - Using the WebView case again, there may be a legitimate reason for another class in Oxide to do OxideQQuickWebViewPrivate::get() in order to access a subset of methods on OxideQQuickWebViewPrivate, but it also exposes the entire API surface of WebViewAdapter as its methods have to be public in order to be accessible from OxideQQuickWebView (unless we make them protected and use a friend statement).
  
  I think we should refactor it all to do this instead (using webview as
  an example):
  
  (sorry if this is not clear)
  
  class OxideQQuickWebView : public QQuickItem {
-  public:
-   ...
-   <public API>
-   ...
+  public:
+   ...
+   <public API>
+   ...
  
-  private:
-   QScopedPointer<OxideQQuickWebViewPrivate> d_ptr;
+  private:
+   QScopedPointer<OxideQQuickWebViewPrivate> d_ptr;
  };
  
  class OxideQQuickWebViewPrivate : public oxide::qt::WebViewProxyClient {
-  public:
-   static OxideQQuickWebViewPrivate* get(OxideQQuickWebView* q);
-   ...
-   <methods available to other code in qt/quick/api>
-   ...
+  public:
+   static OxideQQuickWebViewPrivate* get(OxideQQuickWebView* q);
+   ...
+   <methods available to other code in qt/quick/api>
+   ...
  
-  private:
-   ...
-   <implementation of WebViewProxyClient>
-   ...
+  private:
+   ...
+   <implementation of WebViewProxyClient>
+   ...
  
-   QScopedPointer<oxide::qt::WebViewProxy> proxy_;
+   QScopedPointer<oxide::qt::WebViewProxy> proxy_;
  };
  
  WebViewProxy and WebViewProxyClient are abstract interfaces in
  qt/core/glue, although WebViewProxy has a static create() function.
  
  In qt/core/browser:
  
  WebView : public WebViewProxy {
-  public:
-   ...
-   <methods available to code in qt/core/browser>
-   ...
+  public:
+   ...
+   <methods available to code in qt/core/browser>
+   ...
  
-  private:
-   ...
-   <implementation of WebViewProxy>
-   ...
+  private:
+   ...
+   <implementation of WebViewProxy>
+   ...
  
-   WebViewProxyClient* client_;
+   WebViewProxyClient* client_;
  };
+ 
+ This fixes the above problems (we don't have to expose implementation
+ details between qt/core/browser and qt/core/glue, the WebViewProxy
+ instance is private to OxideQQuickWebViewPrivate and its API isn't
+ exposed to classes that call OxideQQuickWebViewPrivate::get(), and
+ developers don't need to spend time trying to figure out which directory
+ to put code in to)

-- 
You received this bug notification because you are a member of Ubuntu
WebApps bug tracking, which is subscribed to Oxide.
https://bugs.launchpad.net/bugs/1435418

Title:
  Get rid of all code in qt/core/glue

Status in Oxide Webview:
  Triaged

Bug description:
  I've tried to rearrange things to make it clear how code should be
  split between, say, WebViewAdapter in qt/core/glue and WebView in
  qt/core/browser, but the code split still isn't really all that clear
  (even to me). In addition to that,  there's other problems:

  - There's a lot of implementation details exposed between the 2 directories, using private methods and friend statements which all seems a bit wrong.
  - Using the WebView case again, there may be a legitimate reason for another class in Oxide to do OxideQQuickWebViewPrivate::get() in order to access a subset of methods on OxideQQuickWebViewPrivate, but it also exposes the entire API surface of WebViewAdapter as its methods have to be public in order to be accessible from OxideQQuickWebView (unless we make them protected and use a friend statement).

  I think we should refactor it all to do this instead (using webview as
  an example):

  (sorry if this is not clear)

  class OxideQQuickWebView : public QQuickItem {
   public:
    ...
    <public API>
    ...

   private:
    QScopedPointer<OxideQQuickWebViewPrivate> d_ptr;
  };

  class OxideQQuickWebViewPrivate : public oxide::qt::WebViewProxyClient {
   public:
    static OxideQQuickWebViewPrivate* get(OxideQQuickWebView* q);
    ...
    <methods available to other code in qt/quick/api>
    ...

   private:
    ...
    <implementation of WebViewProxyClient>
    ...

    QScopedPointer<oxide::qt::WebViewProxy> proxy_;
  };

  WebViewProxy and WebViewProxyClient are abstract interfaces in
  qt/core/glue, although WebViewProxy has a static create() function.

  In qt/core/browser:

  WebView : public WebViewProxy {
   public:
    ...
    <methods available to code in qt/core/browser>
    ...

   private:
    ...
    <implementation of WebViewProxy>
    ...

    WebViewProxyClient* client_;
  };

  This fixes the above problems (we don't have to expose implementation
  details between qt/core/browser and qt/core/glue, the WebViewProxy
  instance is private to OxideQQuickWebViewPrivate and its API isn't
  exposed to classes that call OxideQQuickWebViewPrivate::get(), and
  developers don't need to spend time trying to figure out which
  directory to put code in to)

To manage notifications about this bug go to:
https://bugs.launchpad.net/oxide/+bug/1435418/+subscriptions