Add Meego Touch based filepicker implementation

RESOLVED FIXED in mozilla9

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Jan Arne Petersen, Assigned: romaxa)

Tracking

(Depends on: 1 bug)

unspecified
mozilla9
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

13.76 KB, patch
heeen
: review+
jbos
: review+
jbos
: feedback+
Details | Diff | Splinter Review
18.28 KB, patch
heeen
: review+
jbos
: review+
Details | Diff | Splinter Review
32.21 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 462546 [details] [diff] [review]
Meego Touch based filepicker implementation
Attachment #462546 - Flags: review?(doug.turner)
(Reporter)

Comment 1

7 years ago
Currently depends on Bug 570115.
Depends on: 570115
(In reply to comment #1)
> Currently depends on Bug 570115.

Why?
(Reporter)

Comment 3

7 years ago
(In reply to comment #2)
> > Currently depends on Bug 570115.
> 
> Why?

Just because it is currently based on the changed nsFilePicker API from patch in bug 570115 (nsFilePicker::ShowNative). It could be rebased of course.
(Assignee)

Comment 4

7 years ago
Comment on attachment 462546 [details] [diff] [review]
Meego Touch based filepicker implementation

I new includes need to be added to [js/src/]config/system-headers
otherwise it fail compile on scratchbox X86
(In reply to comment #3)
> Just because it is currently based on the changed nsFilePicker API from patch
> in bug 570115 (nsFilePicker::ShowNative). It could be rebased of course.

Ah, OK. I'd like to WONTFIX bug 570115, so just wanted to clarify.

Comment 6

7 years ago
Created attachment 466458 [details] [diff] [review]
Updated patch for MeegoTouch file picker implementation

This is an updated version of the patch at attachment #462546 [details] [diff] [review] from comment #0.

This one fixes a problem the old one had: Multiple file pickers could be opened by clicking the button that opens it multiple times (but slower than multi-click interval) before the first file picker is opened. In that case, now the existing file picker is raised to the front.

Updated

7 years ago
Attachment #466458 - Flags: review?(doug.turner)

Comment 7

7 years ago
Comment on attachment 466458 [details] [diff] [review]
Updated patch for MeegoTouch file picker implementation


>+MOZ_ENABLE_CONTENTMANAGER = @MOZ_ENABLE_CONTENTMANAGER@

We should just use the MOZ_ENABLE_MEEGOTOUCH #define.
> SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
> 
> EXTRA_DSO_LDOPTS = \
> 		$(MOZ_COMPONENT_LIBS) \
> 		-lgkgfx \
> 		-lthebes \
> 		$(MOZ_JS_LIBS) \
> 		$(MOZ_QT_LIBS) \
>+		$(MOZ_PLATFORM_MAEMO_LIBS) \
> 		$(GLIB_LIBS) 


I do not understand this change.


>+// Use Qt Tracker API to get attachment data from tracker:
>+#include <QtTracker/Tracker>
>+#include <QtTracker/ontologies/nie.h>
>+#include <QtTracker/ontologies/nfo.h>
>+
>+using namespace SopranoLive;
Attachment #466458 - Flags: review?(romaxa)
Attachment #466458 - Flags: review?(doug.turner)
Attachment #466458 - Flags: review-
(Assignee)

Comment 8

7 years ago
Comment on attachment 466458 [details] [diff] [review]
Updated patch for MeegoTouch file picker implementation

>+MOZ_ENABLE_CONTENTMANAGER = @MOZ_ENABLE_CONTENTMANAGER@
yes we are not using separate flags for each meego related library... just use meego6 ifdef

>+ifeq ($(MOZ_ENABLE_CONTENTMANAGER), 1)
>+MOCSRCS += moc_nsFilePicker.cpp
>+endif

This is not meego or maemo specific... it is just Qt object moc-ization.... it will break build if MOZ_ENABLE_CONTENTMANAGER not defined


>+
>+#if HAVE_VISIBILITY_ATTRIBUTE
>+#  define NSFILEPICKER_EXPORT __attribute__ ((visibility ("default")))
>+#else
>+#  define NSFILEPICKER_EXPORT
>+#endif

there are some macros available already in nscore.h, something like NS_VISIBILITY_DEFAULT, NS_EXTERNAL_VIS
I think it is better to use ready macros here.


>+
>+static QString openFilename(QWidget *parent, const QString &caption,

If I remember correctly it should be like this, according to latest mozilla code style:
 +static QString openFilename(QWidget* parent, const QString& caption,

>+{
>+    QString result;
>+
>+    if (MeegoFilePicker::pickerActive()) {
>+        qDebug() << __FUNCTION__  << ": Ignoring request to open secondary content picker";
I don't really like idea of putting qDebug entries everywhere... they are really annoying on desktop builds and put messages to stdout...

See bug 588186



>+
>+    if (picker) { // Need separate 'if' because picker might have been destroyed by its parent during exec()
>+        if ( picker->hasSelectedFileNames()) {
              ^ - some spaces not  needed  style style...
>+            result = picker->selectedFileNames();



> 
>-    mDialog->selectFile(QString::fromUtf16(mDefault.get()));
>+    if (!selected.isEmpty()) {
>+      QString path = QFile::encodeName(selected);
I think the rest of file is using 4 spaces...


>-void nsFilePicker::InitNative(nsIWidget *parent, const nsAString &title, PRInt16 mode)
>+void nsFilePicker::InitNative(nsIWidget *aParent, const nsAString &aTitle,
>+                              PRInt16 aMode)

Is this change really needed ?, remove all style only changes pls


>+MApplicationPage * MeegoFilePicker::createOpenFilePage()
>+{
>+    QStringList itemType("http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#FileDataObject");
this string is used  many times here, I think it should be const....
also is it documented feature? do we have meego define for this string?
Attachment #466458 - Flags: review?(romaxa) → review-
(Assignee)

Comment 9

7 years ago
Comment on attachment 462546 [details] [diff] [review]
Meego Touch based filepicker implementation

I think this patch is obsolete now...
Attachment #462546 - Flags: review?(doug.turner)
(Reporter)

Comment 10

7 years ago
Comment on attachment 462546 [details] [diff] [review]
Meego Touch based filepicker implementation

Obsolete.
Attachment #462546 - Attachment is obsolete: true

Updated

7 years ago
Depends on: 594368
Created attachment 547673 [details] [diff] [review]
Updated Patch to current version of API

This patch enables the filepicker of Harmattan. For normal Qt the usual Filepicker is still used.
Attachment #466458 - Attachment is obsolete: true
Attachment #547673 - Flags: review?(romaxa)
(Assignee)

Comment 12

6 years ago
Comment on attachment 547673 [details] [diff] [review]
Updated Patch to current version of API

This patch is not against mozilla-central source code...
> +        MOZ_QT_LIBS="-L$QTDIR/lib/ -lQtGui -lQtNetwork -lQtCore -lQtDBus -lQtXml -lQtOpenGL -lQtSparql"

Sounds like this will force linking against lQtSparql which is not existing on desktop...

># User Stefan Hundhammer <Stefan.Hundhammer@xxx.com>
I guess you reworked this patch , and should paste your name here...
(Assignee)

Comment 13

6 years ago
Comment on attachment 547673 [details] [diff] [review]
Updated Patch to current version of API

libqttracker1pre6 - is not available in default image... so this dependency must be removed...
Attachment #547673 - Flags: review?(romaxa) → review-
Created attachment 548142 [details] [diff] [review]
Updated Patch to current version of API - removed qttracker dependency
Attachment #548142 - Flags: review?(romaxa)
Created attachment 548144 [details] [diff] [review]
Updated Patch to current version of API - removed qttracker dependency

Overlooked #12, updated.
Attachment #547673 - Attachment is obsolete: true
Attachment #548142 - Attachment is obsolete: true
Attachment #548144 - Flags: review?(romaxa)
Attachment #548142 - Flags: review?(romaxa)
(Assignee)

Comment 16

6 years ago
Comment on attachment 548144 [details] [diff] [review]
Updated Patch to current version of API - removed qttracker dependency


>-        MOZ_QT_LIBS="-L$QTDIR/lib/ -lQtGui -lQtNetwork -lQtCore -lQtDBus -lQtXml -lQtOpenGL"
>+        MOZ_QT_LIBS="-L$QTDIR/lib/ -lQtGui -lQtNetwork -lQtCore -lQtDBus -lQtXml -lQtOpenGL" 
Any useful change in here?


>diff -r 2be0b2e30b49 debian/control
>--- a/debian/control	Mon Jul 25 12:55:46 2011 +0200
>+++ b/debian/control	Mon Jul 25 13:02:52 2011 +0200
>@@ -2,7 +2,7 @@
> Section: unknown
> Priority: extra
> Maintainer: Oleg Romashin <oleg.romashin@nokia.com>
>-Build-Depends: debhelper (>= 4),  autoconf2.13, libidl-dev, quilt, python (>= 2.4), libqt4-dev, libxext-dev, libxext-dev, libpng12-dev | libpng-dev, libfontconfig1-dev, libcontentaction-dev, sharutils, aegis-builder, libiw-dev, libdbus-1-dev, libdbus-glib-1-dev, libasound2-dev, libxrender-dev, libpango1.0-dev, yasm, libqtm-sensors-dev, libqmsystem2-dev, libmeegotouch-dev
>+Build-Depends: debhelper (>= 4),  autoconf2.13, libidl-dev, quilt, python (>= 2.4), libqt4-dev, libxext-dev, libxext-dev, libpng12-dev | libpng-dev, libfontconfig1-dev, libcontentaction-dev, sharutils, aegis-builder, libiw-dev, libdbus-1-dev, libdbus-glib-1-dev, libasound2-dev, libxrender-dev, libpango1.0-dev, yasm, libqtm-sensors-dev, libqmsystem2-dev, libmeegotouch-dev, libqtsparql-dev 
> Standards-Version: 3.7.3

this is not part of mozilla-central

> Package: fennec
>diff -r 2be0b2e30b49 extensions/spellcheck/hunspell/tests/suggestiontest/Makefile.orig
>--- a/extensions/spellcheck/hunspell/tests/suggestiontest/Makefile.orig	Mon Jul 25 12:55:46 2011 +0200
>+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
>@@ -1,11 +0,0 @@
>-all:
>-	./prepare
>-	./test
>-
>-single:
>-	./prepare2
>-	./test
>-
>-clean:
>-	rm *.[1-5] result.*
>-

garbage?


>+#include <qfile.h>
>+#include <qstringlist.h>
>+#include <qgraphicswidget.h>

I guess it is better to use Qt header wrappers isn't it? like <QGraphicsWidget>


>+    }
>+
>+    if (picker) { // Need separate 'if' because picker might have been destroyed by its parent during exec()
>+        if ( picker->hasSelectedFileNames()) {
             ^ - indent, random whitespaces...
>+            result = picker->selectedFileNames().first();
>+        }

>+
>+static QString openDirectory(QWidget *parent, const QString &caption, const
                                      
(QWidget* parent - this is more close to moz-style

>-            selected = files[0];
>+    #if defined(MOZ_ENABLE_CONTENTMANAGER) && defined(MOZ_ENABLE_MEEGOTOUCH)
>+        MWindow *parentWidget = MApplication::activeWindow();
>+    #else

ifdefs should start from beginning of the line




>+//---------------
>+#if defined(MOZ_ENABLE_CONTENTMANAGER) && defined(MOZ_ENABLE_MEEGOTOUCH)

Does MOZ_ENABLE_CONTENTMANAGER make any sense without meegotouch?


>+
>+        MWindow * parentWindow = MApplication::activeWindow();

MWindow* parentWindow, here and in other places


Also I still see some strange list of applications, instead of Documents/Photos/Media list.
Attachment #548144 - Flags: review?(romaxa) → review-
harmattan always show this strange list of applications... (see search in system grid...) you may use "FileObject" as type - where you see docs, media,... but all emtpy so harmattan does not seem to use that. 

Fixes coming
(Assignee)

Comment 18

6 years ago
Problem is that on android we have also list of applications, but you are able to choose that app and select required file and get path into "input file" element.
Also on harmattan Gallery able to get list of images, media player able to get list of media files, and documents app getting list of documents, so obviously it is somehow possible to do that... can we look inside these apps and check how do they do that?

otherwise I would prefer to keep ugly but working/functional Qt styled filepicker rather than nice harmattan styled dialog which does not allow to do anything...
you can still search for content, ie. type mp3 or image...
(Assignee)

Comment 20

6 years ago
oh... that is not really obvious... can we just pre-search something like "*.jpg" in search field, so it will find all available files in tracker?
And could you remove non-file types from search results?
jep, actually, we just setup a long presearch (first QString() parameter)

Like mp3 mp4 avi pdf doc ogg webm zip jpg jpeg png bmp   it will show them all
a i forgot, space is separator and the user wont see the search string, though I will push fixed patch with that.

stuff like wildcards do not work...
Created attachment 549040 [details] [diff] [review]
This is a bit less issued version...
Attachment #548144 - Attachment is obsolete: true
Attachment #549040 - Flags: review?(romaxa)
(Assignee)

Comment 24

6 years ago
Just tested new version and when I selected some file, it was not inserted into upload input field..... 
Also another issue is that when you remove text from search field it show documents/music et.c clicking on them does not do anything
(In reply to comment #24)
> Just tested new version and when I selected some file, it was not inserted
> into upload input field.....
 
You need to sign your packages with aegis... (see the bugreport about that)


> Also another issue is that when you remove text from search field it show
> documents/music et.c clicking on them does not do anything

See... Thats why I did simply the "default" list...
(Assignee)

Comment 26

6 years ago
> > into upload input field.....
>  
> You need to sign your packages with aegis... (see the bugreport about that)

I have that patch applied, but still not able to get any text/file path into HTML element...
I checked your build. There its really not working, while for mine it does (same device, image,...)

can you post me your mozconfig? How do you build the things? something must be different.
(Assignee)

Comment 28

6 years ago
My mozconfig just that one which is generated by dpkg-buildpackage from hg.meego.com
then make deb -C obj-build...

Also I see stuff implemented here, but it does not work fully... does build from hg.meego.com works for you?
Created attachment 552632 [details] [diff] [review]
updated security access

I looks like the issue was caused through some security system stuff...
With the updated version it works for me now - can you verify?
Attachment #549040 - Attachment is obsolete: true
Attachment #552632 - Flags: review?(romaxa)
Attachment #549040 - Flags: review?(romaxa)
(Assignee)

Comment 30

6 years ago
Comment on attachment 552632 [details] [diff] [review]
updated security access

Ok, I finally got it mostly working

>   <request>
>     <credential name="TrackerReadAccess" />
>     <credential name="TrackerWriteAccess" />
>+    <for path="/usr/local/lib/fennec-8.0a1/fennec" />
Add filter substitution and use @installdir@/@MOZ_APP_NAME@


>+// Extern variables from qfiledialog.cpp deep inside libqt
>+NSFILEPICKER_EXPORT extern _qt_file_open_filename_hook qt_filedialog_open_filename_hook;
>+NSFILEPICKER_EXPORT extern _qt_file_open_filenames_hook qt_filedialog_open_filenames_hook;
>+NSFILEPICKER_EXPORT extern _qt_file_existing_directory_hook qt_filedialog_existing_directory_hook;

Ok, I was looking at this implementation year ago and now and still don't like this hacky approach, and magic with file dialogs...
1) IIUC we are creating normal QFileDialog and then overriding it's view using hacks and displaying ContentManager view on top of it...
   This make stuff unstable, it is easy to get fennec frozen and killed if you move fennec to TS and try to close it in TS.
2) Would not it be better instead of launching QFileDialog and doing terrible hacks on top of it, just create normal Q(GraphicsView)Widget and just integrate Application page into that widget.
3) Also I think it is better to isolate MeegoContent manager impllementation into it's own file (nsMeegoFilePicker or something) and move full implementation into there... including Widget creation et.c

Also don't forget about styling... it seems broken in many places in new implementation
Attachment #552632 - Flags: review?(romaxa) → review-
Created attachment 554130 [details] [diff] [review]
Updated - remove hang
Attachment #552632 - Attachment is obsolete: true
(Assignee)

Comment 32

6 years ago
Created attachment 554305 [details] [diff] [review]
Move MFilePicker to new file and style fixes

Last patch crashes, but now right when we go to background... 
Here I created mechanically reorganized version of previous patch, no hooks, moved into new file, fixed style issues.
Still need to cleanup list of  (some of them obviously not needed). also need to understand logic of Meegotouch window creation and how to simplify it.
(Assignee)

Comment 33

6 years ago
Created attachment 554614 [details] [diff] [review]
Current QFliePicker  part, make it less depndend from QFileDialog
Attachment #554130 - Attachment is obsolete: true
Attachment #554305 - Attachment is obsolete: true
(Assignee)

Comment 34

6 years ago
Created attachment 554615 [details] [diff] [review]
Megoutuch implementation
(Assignee)

Comment 35

6 years ago
Still need to check that first patch did not break old filepicker behavior, multiselection et.c. on desktop
(Assignee)

Comment 36

6 years ago
Comment on attachment 554614 [details] [diff] [review]
Current QFliePicker  part, make it less depndend from QFileDialog

Ok, Multi-file picker seems does not work even with current code...
we should fix that in another bug.
Attachment #554614 - Flags: review?(mozilla)
(Assignee)

Comment 37

6 years ago
Comment on attachment 554615 [details] [diff] [review]
Megoutuch implementation

MEegotouch implementation, moved into separate file, and ifdefed as replacement for QFileDialog
Attachment #554615 - Flags: review?(mozilla)
I'm about to leave for vacation for two weeks and cannot review that before anymore. Sorry.
(Assignee)

Updated

6 years ago
Attachment #554614 - Flags: review?(mozilla)
Attachment #554614 - Flags: review?(doug.turner)
Attachment #554614 - Flags: feedback?(jeremias.bosch)
(Assignee)

Comment 39

6 years ago
Comment on attachment 554615 [details] [diff] [review]
Megoutuch implementation

Someone with fresh eye should look at this impl...
Attachment #554615 - Flags: review?(mozilla) → review?(doug.turner)
Comment on attachment 554615 [details] [diff] [review]
Megoutuch implementation

Looks and works fine for me.
Attachment #554615 - Flags: review+
Comment on attachment 554614 [details] [diff] [review]
Current QFliePicker  part, make it less depndend from QFileDialog

Review of attachment 554614 [details] [diff] [review]:
-----------------------------------------------------------------

Its fine to do it this way. Works as expected.
Attachment #554614 - Flags: review+
Attachment #554614 - Flags: feedback?(jeremias.bosch)
Attachment #554614 - Flags: feedback+
(Assignee)

Updated

6 years ago
Attachment #554614 - Flags: review?(doug.turner) → review?(florian.haenel)
(Assignee)

Comment 42

6 years ago
Comment on attachment 554615 [details] [diff] [review]
Megoutuch implementation

Florian take a look at this with fresh eyes
Attachment #554615 - Flags: review?(doug.turner) → review?(florian.haenel)
Comment on attachment 554615 [details] [diff] [review]
Megoutuch implementation

Review of attachment 554615 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with issues resolved

::: widget/src/qt/nsMFilePicker.cpp
@@ +129,5 @@
> +{
> +    QStringList itemType("http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#FileDataObject");
> +
> +    SelectSingleContentItemPage* page =
> +        new SelectSingleContentItemPage(QString("pdf mp3 mp4 jpg png avi acc ogg doc zip"),

Do we really need to hardcode this list? Is it really guaranteed to be complete? What about html, txt, xls, rar, tar.gz, mkv, flac, ogv...
Can we just allow for all types and let tracker decide what it serves us?

@@ +239,5 @@
> +    picker->mCaption = caption;
> +    picker->exec();
> +
> +    // Need separate 'if' because picker might have been destroyed by its parent during exec()
> +    if (picker->hasSelectedFileNames()) {

does this mean picker could be NULL here? If so we need to do if(picker && picker->...)

@@ +257,5 @@
> +    picker->mCaption = caption;
> +    picker->exec();
> +
> +    // Need separate 'if' because picker might have been destroyed by its parent during exec()
> +    if (picker->hasSelectedFileNames()) {

see NULL comment above

@@ +278,5 @@
> +    picker->mCaption = caption;
> +    picker->exec();
> +
> +    // Need separate 'if' because picker might have been destroyed by its parent during exec()
> +    if (picker->hasSelectedFileNames()) {

see above
Attachment #554615 - Flags: review?(florian.haenel) → review+
Comment on attachment 554614 [details] [diff] [review]
Current QFliePicker  part, make it less depndend from QFileDialog

Review of attachment 554614 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits resolved

::: widget/src/qt/nsFilePicker.cpp
@@ +238,5 @@
> +
> +    switch (mMode) {
> +    case nsIFilePicker::modeOpen:
> +        selected = MozFileDialog::getOpenFileName(parentQWidget, mCaption,
> +                                                  directory.get(), filters.join(";;"));

I think single ";" should be enough here?

@@ +246,5 @@
> +        }
> +        break;
> +    case nsIFilePicker::modeOpenMultiple:
> +        files = MozFileDialog::getOpenFileNames(parentQWidget, mCaption,
> +                                                directory.get(), filters.join(";;"));

see above
Attachment #554614 - Flags: review?(florian.haenel) → review+
the second patch is giving me build errors:


invoking make to create js-config script
make[3]: Entering directory `/home/fhae/upstream/mozilla-central/obj-fn-qt-arm/js/src'
make[3]: `js-config' is up to date.
make[3]: Leaving directory `/home/fhae/upstream/mozilla-central/obj-fn-qt-arm/js/src'
config/autoconf.mk is unchanged
make[2]: Leaving directory `/home/fhae/upstream/mozilla-central'
TEST-UNEXPECTED-FAIL | check-sync-dirs.py | build file copies are not in sync
TEST-INFO | check-sync-dirs.py | file(s) found in:               /home/fhae/upstream/mozilla-central/js/src/config
TEST-INFO | check-sync-dirs.py | differ from their originals in: /home/fhae/upstream/mozilla-central/config
TEST-INFO | check-sync-dirs.py | differing file:                 ./system-headers
In general, the files in '/home/fhae/upstream/mozilla-
central/js/src/config' should always be exact copies of originals in
'/home/fhae/upstream/mozilla-central/config'.  A change made to one should
also be made to the other.  See 'check-sync-dirs.py' for more details.
make[1]: *** [realbuild] Error 1
make[1]: Leaving directory `/home/fhae/upstream/mozilla-central'
make: *** [build] Error 2
(Assignee)

Comment 46

6 years ago
Created attachment 559998 [details] [diff] [review]
Fixed nits, combined patch
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
(Assignee)

Comment 47

6 years ago
try build looks good enough
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=79cc02cd81d7
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/131496f18137
Keywords: checkin-needed
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/131496f18137
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.