Reapply #75

gyorokpeter · 2022-02-18T15:52:57Z

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

linux-foundation-easycla · 2022-02-18T15:53:03Z

The committers are authorized under a signed CLA.

✅ gyorokpeter (ef9a35e, df31c1f, 6b8acea, edab6d3, f198cef, 01adf01)

dzmipt · 2022-02-19T11:21:03Z

src/main/studio/kdb/Config.java

-        return p.getProperty("auth", DefaultAuthenticationMechanism.NAME);
+        String result = p.getProperty("auth", null);
+        if (result == null) result = System.getenv().getOrDefault("KDB_STUDIO_AUTH_METHOD", DefaultAuthenticationMechanism.NAME);
+        return result;


The env. variable will be used until the user opens Setting dialog. If the user change the env. variable after that, it wouldn't take any effect. Is it desired behaviour?

The original use case is to just override the default for the first time the user starts the app, then it should be saved in the settings. This saves the user the step of changing the default auth method manually (or worse, not realizing that there is a way to set the default and so setting it on each server), and eliminates the support requests from users failing to do so.

dzmipt · 2022-02-19T11:35:16Z

src/main/studio/ui/StudioPanel.java

                    }
+                    TabPanel tab = new TabPanel(panel, queryResult, null);


Console view is a great idea. However UX is confusing... If I drag&drop tabs, then there is no way to match console to visual (Table/List) tab.

I prefer a concept that one result produces one tabs. And inside the tab there is a switcher to select either visual or console view.

That would be a good future enhancement, for now the goal is to just restore the "lost" functionality so can we can do an internal release ASAP.
#78

dzmipt · 2022-02-20T09:32:50Z

src/main/studio/kdb/Config.java

}
+

I wanted to simplify definition of properties in the Config (however i didn't finish with all the config)... This (old) approach would result that we need to copy/paste the code for any future getResultPaneFont(), getAnyOtherFont(), etc.

My idea is to implement getXXX and setXXX once for a type and then adding a new property would one line.

I started to do this for Font.. However now I see that it wasn't finished... You can complete yourself. Or I can raise a PR later if you want...

Here is how to complete... Restore my version of getFont and setFont. And you can use CONFIG.getFont(Config.FONT_EDITOR) and CONFIG.setFong(Config.FONT_EDITOR, font). But in addition you need to fix:

Config.configDefault: else if (type == ConfigType.FONT) { Font font = (Font) defaultValue; configDefault(key + ".size", ConfigType.INT, font.getSize()); configDefault(key + ".name", ConfigType.STRING, font.getName()); configDefault(key + ".style", ConfigType.ENUM, FontStyle.getStyle(font.getStyle())); }else if (type == ConfigType.FONT) { Font font = (Font) defaultValue; configDefault(key + ".size", ConfigType.INT, font.getSize()); configDefault(key + ".name", ConfigType.STRING, font.getName()); configDefault(key + ".style", ConfigType.ENUM, FontStyle.getStyle(font.getStyle())); } Config.FontStyle: public static FontStyle getStyle(int fontStyle) { return FontStyle.values()[fontStyle]; }

dzmipt · 2022-02-20T09:34:50Z

src/main/studio/ui/EditorPane.java

@@ -129,4 +129,9 @@ private void setBorder(JComponent component) {
                )
        );
    }
+
+    public void setFont2(Font font) {   //don't call this setFont, it leads to an error


Technically such method is not required as it is possible to execute pane.getTextArea().setFont().
Or if you think we need to encapsulate the logic here, I would suggest to name the method setTextAreaFont (tbh, setFont2 looks confusing to me)

dzmipt · 2022-02-20T09:38:14Z

src/main/studio/ui/StudioPanel.java

-
+        Font font = new Font(dialog.getFontName(), Font.PLAIN, dialog.getFontSize());
+        Config.getInstance().setFont(font);
+        activePanel.editor.getPane().setFont2(font);


There is CONFIG variable - the code above use it instead of Config.getInstance()

Why didn't you modify the font for other opened panes and frames ?

Font size of line numbers are not changed. So it will be inconsistent result on previously opened panes and new panes.

I tried this:

CONFIG.setFont(Config.FONT_EDITOR, font); for(StudioPanel panel: allPanels) { panel.editor.getPane().setTextAreaFont(font); }

and

public void setTextAreaFont(Font font) { //don't call this setFont, it leads to an error textArea.setFont(font); scrollPane.getGutter().setLineNumberFont(font); }

But this results in the currently open tab getting the updated line numbers and spacing, while the other tabs still having the old size for the numbers and the old line spacing but with the actual font of the text updated. Any idea what I'm doing wrong?

You iterate over panels (which are frames) but not tabs. I think you need to add the logic into refreshEditorSetting()

dzmipt · 2022-02-20T09:41:09Z

src/main/studio/ui/SettingsDialog.java

+        GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
+        cbFontName = new JComboBox(ge.getAvailableFontFamilyNames());
+        cbFontName.getModel().setSelectedItem(Config.getInstance().getFontName());
+


I thought to use fontchooser from org.drjekyll (which is already added as a dependency in the build.gradle).

dzmipt · 2022-02-20T09:43:34Z

src/main/studio/kdb/Config.java

@@ -306,6 +306,33 @@ private void init(String filename, Properties properties) {
            }
        }

+        if (!Files.exists(file) && System.getProperty("os.name").startsWith("Windows")) {
+            System.out.println("Config not found in userprofile. Trying legacy path.");


why not log.info ?

Does that print to stdout without any action from the user (e.g. providing log configuration)?

System.out and System.err are trapped and also sent to Log4J (this is the reason of log4j-iostreams dependency). However this looses information about context (class name). So it looks more naturally to use logging.

Then how would I make sure that the log.info lines are actually sent to the console?

It is defined in log4j2.xml configuration

<AppenderRef ref="Console"/> <AppenderRef ref="AsyncStudioLog"/>

Everything which is sent to log4j is logged to both ~/.studioforkdb/log/studio.log as well as to the console.

Also System.out and System.err are hooked to be used by log4j and to be logged in the same way (with the exception that context will be lost...)

Strange because when I tried to invoke log.info unconditionally nothing appeared on the console, while exception traces do appear. There is also no "log" directory in %USERPROFILE%/.studioforkdb (where the .properties files do appear normally).

It turns out that log4j2.xml is not in the classpath. And this is because of the build.gradle:

srcDirs 'src/main','images' include 'log4j2.xml'

It loads all resources from "include" (this one and all below) under src/main and images folder.

So one of the possible solution is to move log4j2.xml into src/main folder.

It is assumed that you should have studio.log and query.log files under ~/.studioforkdb/log/

Thanks, moving the file fixes both issues.

dzmipt · 2022-02-20T09:44:53Z

src/main/studio/kdb/Config.java

@@ -306,6 +306,33 @@ private void init(String filename, Properties properties) {
            }
        }

+        if (!Files.exists(file) && System.getProperty("os.name").startsWith("Windows")) {


We have Util.WINDOWS

dzmipt · 2022-02-20T09:45:13Z

src/main/studio/kdb/Config.java

+            } catch (IOException e) {
+                //ignore
+            }
+            System.out.println("Old path: "+oldpath);


dzmipt · 2022-02-20T10:07:51Z

src/main/studio/kdb/Config.java

@@ -306,6 +306,33 @@ private void init(String filename, Properties properties) {
            }
        }

+        if (!Files.exists(file) && System.getProperty("os.name").startsWith("Windows")) {
+            System.out.println("Config not found in userprofile. Trying legacy path.");
+            //Old Java versions returned a different place for user.home on Windows.


Out of curiosity - what java version returns Desktop for "user.home" ? I tried to Google, and found that someone experience sporadic problems - but the location was different.
https://coderanch.com/t/517573/java/System-getProperty-user-home-unreliable

May be instead of such code, user can copy the config to his/her user home?

BTW, also you can access native Windows API where I believe should be method to load from Registry. See WindowsAppUserMode class for an example to access Windows native functions.

"user can copy the config to his/her user home"
That is the very motivation of this change, to NOT require the user to do anything. We were getting a lot of support requests about server lists disappearing after an upgrade, despite there being docs available about where the settings are and where they should be copied.

As for the Java version change, it was somewhere between 1.6.0 and 1.8.0.

dzmipt · 2022-02-28T17:42:20Z

src/main/studio/kdb/Config.java

+    public Object serverTreeToObj(ServerTreeNode root) {
+        //converts the server tree to an object that can be saved into JSON
+        LinkedHashMap<String,Object> result = new LinkedHashMap<>();
+        result.put("name", root.getName());


Such encoding assumes that leaf node names are unique which is not guaranteed. I assume import/export logic would be broken if i have two or more severs with the same name but in the different location in the tree

In fact server names should be unique otherwise there is a chance that there will be a clash when moving them under the same parent. There was already a feature request to enforce this which I didn't get around to implement yet but looks like I will have to do it now.

My view that we need to enforce uniqueness of full name, I know that people already have configuration with the same names under different locations in the server tree. If there are use-cases when uniqueness of full name is not guaranteed, i think they should be fixed.

I will add this as a future enhancement since it is too involved to deal with in this PR which is to reapply existing changes, i.e. this code is already being run by users who are not reporting any problems with it.
#81

dzmipt · 2022-02-28T17:51:03Z

src/main/studio/ui/ServerList.java

-            TreePath treePath = new TreePath(selNode.getPath());
-            tree.scrollPathToVisible(treePath);
-            tree.setSelectionPath(treePath);
+    private void fileDialog(String title, boolean isSave, Consumer<File> fileOp) {


This logic duplicates StudioPanel.chooseFile which also has the following advantages (comparing to this implementation):

add file extension (e.g. when I chose export, typed in "test" with JSON file filter, I got test file while I would expect test.json)

persist folder location and the dialog size

I guess the code of chooseFile at that time was not that advanced. I will refactor this.

dzmipt · 2022-02-28T18:04:17Z

src/main/studio/ui/ServerList.java

        }
-        refreshServers();
+        addExistingNode(selNode, pickedUpNode, location);
+    }


From UX perspective:

I would implement D&D

If we implement pick-up solution, I would have visual indication of the picked up item (font/icon)

There is Server/Import servers from QPad menu item. IMHO I would put import/export json somewhere close into the main menu

When I picked and moved a server, comboServer didn't refresh. And as a result I wasn't able to select the server from the comboBox

#82 created for drag and drop as it wouldn't fit the scope of this PR.

I tried to change the label of the tree node when a node is picked up but looks like how things behave doesn't reflect what is in the docs. So I'll leave that for now. If drag and drop is implemented this would be unnecessary anyway.

(cherry picked from commit 8616e2a)

dzmipt · 2022-03-01T20:24:07Z

Can you please elaborate what clash you mean? It seems to be OK to have structure like EnvA/rdb/stock EnvB/rdb/stock вт, 1 мар. 2022 г., 19:08 gyorokpeter ***@***.***>:

…

***@***.**** commented on this pull request. ------------------------------ In src/main/studio/kdb/Config.java <#75 (comment)>: > @@ -435,6 +438,148 @@ public void save() { } } + public Object serverTreeToObj(ServerTreeNode root) { + //converts the server tree to an object that can be saved into JSON + LinkedHashMap<String,Object> result = new LinkedHashMap<>(); + result.put("name", root.getName()); In fact server names should be unique otherwise there is a chance that there will be a clash when moving them under the same parent. There was already a feature request to enforce this which I didn't get around to implement yet but looks like I will have to do it now. — Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG76OVXZBQEZ6SDQY2JYIRDU5Y6GBANCNFSM5OYKFZDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

gyorokpeter · 2022-03-02T07:19:51Z

If you were to pick up the first "rdb" and and drop it into the "EnvB/rdb" folder you would end up with a clash.
The fact that there is no validation for duplicate server names is already causing problems for some users even without the server tree or export feature (see #22 ) so implementing that seems to be the best solution.

gyorokpeter requested a review from punx120 February 18, 2022 15:52

dzmipt reviewed Feb 19, 2022

View reviewed changes

dzmipt reviewed Feb 20, 2022

View reviewed changes

gyorokpeter force-pushed the reapply branch 2 times, most recently from c7b996d to e3b8341 Compare February 22, 2022 12:49

dzmipt reviewed Feb 28, 2022

View reviewed changes

dzmipt and others added 7 commits March 1, 2022 16:00

do not show dialog box if the server return kdb signal

413713e

(cherry picked from commit 8616e2a)

reapply: allow configs to be read from legacy paths

dea26df

reapply: add back Console View for lists, dicts and tables

c8d4d12

reapply: allow setting default method via environment variable

3991cf2

reapply: don't show backtick before error messages

368c8fc

reapply: add font configuration

559c800

move log4j2.xml under main to ensure it is included in the jar

07658b9

Peter Gyorok added 3 commits March 3, 2022 11:22

block duplicate server names

33d9153

don't report failure to set taskbar icon if it's not supported

40e7dea

reapply: add server list import/export, moving servers between folders

48dbd44

gyorokpeter force-pushed the reapply branch from 9b02c58 to 48dbd44 Compare March 3, 2022 13:28

gyorokpeter merged commit a53ec90 into finos:main Mar 14, 2022

alvin-c-shih mentioned this pull request Apr 10, 2022

2022-03-14 - KDB Project Call Meeting Minutes finos/kdb#69

Closed

2 tasks

gyorokpeter deleted the reapply branch June 28, 2022 09:48

Reapply #75

Reapply #75

Conversation

gyorokpeter commented Feb 18, 2022

linux-foundation-easycla bot commented Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorokpeter Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzmipt commented Mar 1, 2022 via email

gyorokpeter commented Mar 2, 2022

linux-foundation-easycla bot commented Feb 18, 2022 •

edited

Loading

gyorokpeter Feb 23, 2022 •

edited

Loading