Skip to content

Fix Table View group buttons #154 - #155

Merged
jdaguil merged 2 commits into
developfrom
154-fix-table-view-group-buttons
Aug 18, 2017
Merged

jdaguil merged 2 commits into
developfrom
154-fix-table-view-group-buttons

Conversation

@johnmhoran

Copy link
Copy Markdown
Member

@jdaguil Re the display problem with the Table View column-group buttons, the show option of the Buttons extension's colvisGroup button type is working properly, but for reasons I’ve not been able to pin down, the hide option fails.

As a result, when a user selects a column-group button, no columns are hidden. On initial load, all columns are displayed, which means that unless the user individually removes columns using the Column visibility button, clicking any of the 3 column-group buttons -- Copyright info, License info and Origin info -- does not change the column display.

These buttons last worked in Release 1.1.0-beta.3 (released 1/11/17). Our show/hide code was contained in scancode.js (all of this inside $(document).ready(function)...:

                {
                    // Show only copyright columns
                    extend: 'colvisGroup',
                    text: 'Copyright info',
                    show: $.map(COPYRIGHT_COLUMNS, function(column, i) {
                        return column.name + ":name";
                    }),
                    hide: $.map($(ScanData.TABLE_COLUMNS).not(COPYRIGHT_COLUMNS),
                        function(column, i) { return column.name + ":name"; })
                },

In the next release, Release v2.0.0-rc1: First v2.0.0 release candidate (released 4/21/17), the show/hide code was contained (as it is now) in aboutCodeDataTables.js (all inside class AboutCodeDataTable {...} ):

                {
                    // Show only copyright columns
                    extend: "colvisGroup",
                    text: "Copyright info",
                    show: AboutCodeDataTable.COPYRIGHT_GROUP
                        .map((column) => `${column.name}:name`),
                    hide: $(AboutCodeDataTable.TABLE_COLUMNS)
                        .not(AboutCodeDataTable.COPYRIGHT_GROUP)
                        .map((column) => `${column.name}:name`)
                },

As noted above, the show option works while the hide option does not work at all -- and commenting out the .not selector has no effect. The console shows no error.

Curiously, if we adapt the Release 1.1.0-beta.3 code, the first line works, though the .not selector does not, resulting in the removal of all columns:

                    hide: $.map($(AboutCodeDataTable.TABLE_COLUMNS)
                        .not(AboutCodeDataTable.COPYRIGHT_COLUMNS),
                        function(column, i) { return column.name + ":name"; })

==Alternative Solution==

I’ve done extensive research and tried a wide range of syntax variations using our current colvisGroup jQuery hide code as well as the earlier approach, thus far without success -- with one exception. (This approach could also be applied to issue #91 Add a Package info group button to Table View, but I’ll defer tackling that issue until we’ve resolved this one.)

Rather than starting with the full set of columns AboutCodeDataTable.TABLE_COLUMNS and trying to exclude those columns we want to remain visible, I’ve modified several of our existing static get variables and added several new ones -- including NOT_COPYRIGHT_GROUP(), NOT_LICENSE_GROUP() and NOT_ORIGIN_GROUP() . Using these in a simplified hide option codeblock achieves the desired functionality and does not appear to create any new problems. An example:

                    hide: AboutCodeDataTable.NOT_COPYRIGHT_GROUP
                        .map((column) => `${column.name}:name`)

This solution is a bit more complex (and less elegant) than the approach that worked in Release 1.1.0-beta.3 and it would not surprise me if there’s a way to adapt that pattern to our new class AboutCodeDataTable {...} structure. I look forward to your thoughts and suggestions.

  * Replace colvisGroup jQuery “hide” code with simple call to new
    “static get” variables.

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@jdaguil

jdaguil commented Aug 16, 2017

Copy link
Copy Markdown
Contributor

@johnmhoran We should avoid hard coding inverse groups of groups we have already defined.

After reviewing I've discovered two problems:

  1. not doesn't work because it uses the in operator, and it seems the in operator does not work for objects inside of an array. For example:
   x = {"a":1}
   x in [x] // returns false instead of true

However, indexOf does work for objects inside an array
[x].indexOf(x) > 0 // returns the index which is 0. if not found would return -1
So we could use indexOf instead of not

hide: AboutCodeDataTable.TABLE_COLUMNS
    .filter((column) => AboutCodeDataTable.COPYRIGHT_GROUP.indexOf(column) < 0)
    .map((column) => `${column.name}:name`)
  1. The other issue is that the static get methods return a new array of new objects every time, so even indexOf was not working. For example:
// returns false because every call to the static get method recreates the objects
AboutCodeDataTable.COPYRIGHT_GROUP[0] == AboutCodeDataTable.COPYRIGHT_GROUP[0] 

This can be fixed by replacing the static get method for example https://github.com/nexB/aboutcode-manager/blob/develop/assets/js/aboutCodeDataTables.js#L269 with

AboutCodeDataTable.COPYRIGHT_COLUMNS =
    [
            {
                "data": function (row, type, val, meta) {
                    return row.copyright_statements.map(statements => {
                        return statements.join("<br/>")
                    }).join("<hr/>");
                },
                "title": "Copyright Statements",
                "name": "copyright_statements",
                "bar_chart_class": "bar-chart-copyrights"
            },
            ......

The above needs to be outside of the class definition (I would put it below the class).

@johnmhoran

Copy link
Copy Markdown
Member Author

Thanks @jdaguil . It would be very helpful to me if we could discuss your analysis so I can understand how I could have reached a similar conclusion and the details of how your solution works.

  * Static getters replaced with arrays defined outside the
    AboutCodeDataTable class.
  * .not selector replaced with .filter/.indexOf.

Signed-off-by: John M. Horan <johnmhoran@gmail.com>
@johnmhoran

Copy link
Copy Markdown
Member Author

@jdaguil @pombredanne Almost ready to push my latest commit, but it took me two attempts to successfully run the build script:

Thu Aug 17, 2017 02:40 PM  /c/code/nexb/dev/aboutcode-manager JMH (154-fix-table-view-group-buttons)
$ python build.py
DEBUG: get_version: tag: 2.0.0-rc3 distance: 114 commit: 1e4f3ee dirty: False
DEBUG: get_version: dirty, using timestamp: 2.0.0-rc3.post114.1e4f3ee

#############################################################
=> BUILDING AboutCode App release: 2.0.0-rc3.post114.1e4f3ee
platform: Windows-10-10.0.15063 sys.platform: win32
using NPM bin at: C:\code\nexb\dev\aboutcode-manager\node_modules\.bin
Traceback (most recent call last):
  File "build.py", line 303, in <module>
    build()
  File "build.py", line 206, in build
    os.makedirs(build_dir)
  File "C:\Python27\lib\os.py", line 157, in makedirs
    mkdir(name, mode)
WindowsError: [Error 5] Access is denied: u'dist'

Thu Aug 17, 2017 02:40 PM  /c/code/nexb/dev/aboutcode-manager JMH (154-fix-table-view-group-buttons)
$ python build.py
√ Rebuild Complete
Packaging app for platform win32 x64 using electron v1.4.0
Wrote new app to dist\AboutCode-Manager-win32-x64
DEBUG: get_version: tag: 2.0.0-rc3 distance: 114 commit: 1e4f3ee dirty: False
DEBUG: get_version: dirty, using timestamp: 2.0.0-rc3.post114.1e4f3ee

#############################################################
=> BUILDING AboutCode App release: 2.0.0-rc3.post114.1e4f3ee
platform: Windows-10-10.0.15063 sys.platform: win32
using NPM bin at: C:\code\nexb\dev\aboutcode-manager\node_modules\.bin
Running electron-rebuild...
Running command: u'C:\\code\\nexb\\dev\\aboutcode-manager\\node_modules\\.bin\\electron-rebuild'...
Running electron-packager...
Running command: u'C:\\code\\nexb\\dev\\aboutcode-manager\\node_modules\\.bin\\electron-packager . AboutCode-Manager --prune --ignore=thirdparty/* --ignore=dist/* --ignore=/\\.idea --ignore=/\\.gitignore --ignore=/test --ignore=/tmp --ignore=/bower.json --platform=win32 --arch=x64 --icon=assets/app-icon/win/aboutcode_256x256.ico --version=1.4.0 --out=dist --asar=true --overwrite=true --win32metadata.CompanyName="https://AboutCode.org" --win32metadata.ProductName="AboutCode-Manager"'...
Build complete: building release archives...

##################################################
AboutCode App BUILD completed with these archives:
    AboutCode-Manager-windows-x64-2.0.0-rc3.post114.1e4f3ee.zip size: 82267777
##################################################


Thu Aug 17, 2017 02:43 PM  /c/code/nexb/dev/aboutcode-manager JMH (154-fix-table-view-group-buttons)
$

@johnmhoran

Copy link
Copy Markdown
Member Author

The executable runs well, no errors.

@pombredanne

Copy link
Copy Markdown
Member

@johnmhoran we do not support building on gitbash on Windows.

@johnmhoran

Copy link
Copy Markdown
Member Author

@pombredanne On Windows 10, I get similar behavior when using Git Bash to checkout the branch I want to build, opening the Windows command prompt, navigating to the project directory, and running python build.py. Ditto if I run python build.py from Visual Studio Code's integrated terminal. In all 3 cases, the .exe seems to run fine, no console errors.

@pombredanne

pombredanne commented Aug 18, 2017

Copy link
Copy Markdown
Member

@johnmhoran does it build on appevyor without errors? and is the appevyor build functional?

WindowsError: [Error 5] Access is denied: u'dist'

You seem to have some permissions problems on you machine that you need to resolve first IMHO

@johnmhoran

Copy link
Copy Markdown
Member Author

@pombredanne Just checked my last commit result on AppVeyor (from yesterday afternoon) -- there are a series of npm warnings (2 re fsevents, the rest re sqlite3), but no errors. The build succeeded, and after extracting the .zip the .exe runs fine, no errors.

@jdaguil

jdaguil commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

LGTM!

@jdaguil
jdaguil merged commit c920f51 into develop Aug 18, 2017
@jdaguil
jdaguil deleted the 154-fix-table-view-group-buttons branch August 18, 2017 20:36
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.

3 participants