You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "mbien (via GitHub)" <gi...@apache.org> on 2024/03/04 02:43:12 UTC
[PR] Git history and diff view improvements [netbeans]
mbien opened a new pull request, #7128:
URL: https://github.com/apache/netbeans/pull/7128
Lets try to make the git history UI a bit more useful. This doesn't fix the advanced search or its 24 branch limit, this is purely UI/UX.
screenshot:
diff view:
- vertical / horizontal layout switcher
- removed loading screen to make the diff load faster :)
- when switching between diffs (eg using the arrows) the UI state is copied to the new diff (text/graphical mode, divider value etc)
- git annotation colors for files
- commit icon instead of folder icon
summary view:
- fixes https://github.com/apache/netbeans/issues/5680
- made the links more obvious
- added collapse msg link which was missing
- merge commits are a little bit desaturated and not rendered in bold
quick search filter:
- implemented all-filter and made it the default
- highlighting is now consistently working in the relevant columns
- even in tooltips
- some other bug fixes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1532923562
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
i tested both again and although both aren't ideal I do prefer the arrows, they are much easier to click on and I do think it looks better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1531585909
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
I wasn't sure about that one myself. But the `...` looked tiny and I initially misunderstood it that it meant that the content didn't fit into the cell and/or the layout was broken. I have no hard feelings about this, but would be curious what others prefer - I can change it back thats no problem.
Unicode has lots of symbols these days, I am wondering if there is something else which intuitively makes sense as something you can click on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1532833160
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
I would be careful with such predictions:
```
matthias@enterprise:~$ mkdir test
matthias@enterprise:~$ cd test
matthias@enterprise:~/test$ touch '<b>filename with (broken) <br>html styling'
matthias@enterprise:~/test$ ls
'<b>filename with (broken) <br>html styling'
matthias@enterprise:~/test$
```
Yes - this broken HTML, given the age of the HTML parser of swing and the codebases relying on established behavior, I suspect, that it will happily chew this. Given that html and the state of its parsers is a royal mess, I would never trust a html parser to reject anything (regrettably my last hope xhtml died more or less).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536693538
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
right, thats how input sanitization works. Validation rejects invalid input, sanitization encodes/cleans it up to make it still somehow useable.
https://javadoc.io/static/org.owasp.encoder/encoder/1.2.3/org/owasp/encoder/Encode.html
> Which html renderer is used then?
NB uses this some components, e.g the tree part of the custom tree table:
https://github.com/apache/netbeans/blob/master/platform/openide.awt/src/org/openide/awt/HtmlRenderer.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "lkishalmi (via GitHub)" <gi...@apache.org>.
lkishalmi commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536226785
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
Can't we embed real buttons there?
Maybe a "Hamburger" menu symbol?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1531601866
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
quick experiment showed that unicode symbols are probably not worth the trouble due to font issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536604433
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
this column actually doesn't use the JDK html renderer but it doesn't hurt to escape it too. We should consider to upgrade input cleanup to some standard lib like OWASP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536685499
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
> this column actually doesn't use the JDK html renderer but it doesn't hurt to escape it too
Which html renderer is used then?
> We should consider to upgrade input cleanup to some standard lib like OWASP.
No we should not.
You can't "cleanup" input in a sane way or only very limited. What you can do is to limit your input to values that match your target domain. For example if you expect a quantity you can limit inputs to "NUMBER UNIT" pairs or you can expect HTML with only a limited subset of allowed elements or attributes.
This works for interactive use where the user can validate the final result (if you filter) or adjust the input according to the validation feedback. But we have no interaction here. The data (filename, comment, author name, ....) is provided as is and needs to be rendered as is.
Cleaning up input for unconstraint domains also does not work. Consider: https://xkcd.com/327/. You can argue, that input sanitizing should have caught that, but then I would ask: On which basis do you define the domain "firstname of a person". Somewhere on this planet that first name will be valid and when the person tries to enter your country and some point you will need to deal with that fact.
What is more input sanitizing can't consider the output format. For example for use in HTML the string `Robert'); DROP TABLE Students;--` is perfectly ok, for use in SQL it must be escaped. There is no need for OWASP, because escaping will do the right thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1532800905
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
RevisionNode is for the diff view tree. It has only commit hashes or file names which is both compatible with html.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#issuecomment-1979150146
this is still a little bit WIP. I want to check if I can implement a file filter with highlighting etc, and also improve how the table behaves in single-file-history mode (https://github.com/apache/netbeans/issues/5680#issuecomment-1979131388)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#issuecomment-2016431698
things I haven't implemented:
- I wanted to add file type icons to the diff view but couldn't find an elegant way of doing this
- diff view file name column has no quicksearch highlighting due to the way how the tree works, I left a comment in-code
- I haven't touched the file history window at all otherwise this PR would be too large. It doesn't listen to hotkeys etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1531601866
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
quick experiment showed that unicode symbols are probably not worth the trouble due to font issues / portability
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1530865426
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
I had serious trouble to understanding this. I saw this:
and wondered: Is this a bug? A rename without a target? What does NetBeans want to tell me? Just by luck I clicked on this and then I understood. The ellipsis symbol is not perfect, but to me it captures the intended meaning better.
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -157,17 +193,22 @@ public PropertyEditor getPropertyEditor() {
}
}
- private static String highlight (String author, String needle, String bgColor, String fgColor) {
+ private static String escape(String text) {
+ return text.replace("<", "<");
Review Comment:
NetBeans has a more complete implementation that is already being used in `DiffTreeTable`
```suggestion
try {
return XMLUtil.toElementContent(text);
} catch (CharConversionException e1) {
Logger.getLogger(RevisionNode.class.getName()).log(Level.INFO, "Can not HTML escape: ", text); //NOI18N
}
```
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
```suggestion
@Override
public String getHtmlDisplayName() {
if (isCommitNode()) {
return "<b>" + escape(getName()) + "</b>";
} else {
String c = annotationColorForAction(event.getAction());
return c != null ? "<font color=" + escape(c) + ">" + escape(getName()) + "</font>" : escape(getName());
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536693538
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
https://javadoc.io/static/org.owasp.encoder/encoder/1.2.3/org/owasp/encoder/Encode.html
> Which html renderer is used then?
what NB uses for some components:
https://github.com/apache/netbeans/blob/master/platform/openide.awt/src/org/openide/awt/HtmlRenderer.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#issuecomment-2019174086
going to squash/rebase to fewer commits and make this ready to merge soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1532833160
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
I would be careful with such predictions:
```
matthias@enterprise:~$ mkdir test
matthias@enterprise:~$ cd test
matthias@enterprise:~/test$ touch '<b>filename with (broken) <br>html styling'
matthias@enterprise:~/test$ ls
'<b>filename with (broken) <br>html styling'
matthias@enterprise:~/test$
```
Yes - this broken HTML, given the age of the HTML parser of swing and the codebases relying on established behavior, I suspect, that it will happily chew this. Given that html and the state of its parsers is a royal mess, I would never trust a html parser to reject anything (regrettably my last hope xhtml died more or less).
Edit: I did not even try to do insane things like directly modifiying the git trees in memory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#issuecomment-1990655432
I added another commit which removes the branch filter of the toolbar. The reason for that is essentially https://github.com/apache/netbeans/pull/6594 which never got fully resolved.
The branch filter in the toolbar filtered the results after the `git log` command ran. This requires to have all branch information in the revisions, if some branches are missing, it is likely that the filter will remove too many commits (it often would show no results at all in larger repos). Removing the branch filter fixes it.
Secondly: there is a second branch filter in the search options section which runs during `git log` and is therefore not affected. Having two branch filters in the same UI is also a bit questionable, it confused me quite often tbh even when it worked. The typical usecase is to view the history of the checked out branch which is the default anyway.
toolbar branch filter removed with search section expanded:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536598970
##########
ide/versioning.util/src/org/netbeans/modules/versioning/history/SummaryCellRenderer.java:
##########
@@ -714,16 +735,20 @@ private String prepareText (String text) {
private class EventRenderer extends JPanel implements ListCellRenderer {
private boolean lastSelection = false;
+ private String lastSearch = null;
+ private int lastShowingFiles = -1;
private final JLabel pathLabel;
private final JLabel actionLabel;
private final JButton actionButton;
private String id;
private final String PATH_COLOR = getColorString(lessInteresting(UIManager.getColor("List.foreground"), UIManager.getColor("List.background"))); //NOI18N
+ private final String SEARCH_COLOR_BG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Background), Color.BLUE)); //NOI18N
+ private final String SEARCH_COLOR_FG = getColorString((Color) Objects.requireNonNullElse(searchHiliteAttrs.getAttribute(StyleConstants.Foreground), UIManager.getColor("List.foreground"))); //NOI18N
public EventRenderer () {
pathLabel = new JLabel();
actionLabel = new JLabel();
- actionButton = new LinkButton("..."); //NOI18N
+ actionButton = new LinkButton("=>"); //NOI18N
Review Comment:
those are essentially stripped down buttons already. Rendering an icon would probably be possible. I am just not someone who makes icons + I am also not so sure if it would actually look better / be worth the effort, since the UI is consistently using hyperlinked text for navigation in that window.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#discussion_r1536693538
##########
ide/git/src/org/netbeans/modules/git/ui/history/RevisionNode.java:
##########
@@ -78,31 +94,59 @@ public RevisionNode(RepositoryRevision.Event revision, SearchHistoryPanel master
initProperties();
}
- RepositoryRevision getContainer() {
- return container;
- }
-
RepositoryRevision.Event getEvent() {
return event;
}
+ @Override
+ public String getHtmlDisplayName() {
+ if (isCommitNode()) {
+ return "<b>"+getName()+"</b>";
+ } else {
+ String c = annotationColorForAction(event.getAction());
+ return c != null ? "<font color="+c+">"+getName()+"</font>" : getName();
+ }
+ }
Review Comment:
right, thats how input sanitization works. Validation rejects invalid input, sanitization encodes/cleans it up to make it still somehow useable.
https://javadoc.io/static/org.owasp.encoder/encoder/1.2.3/org/owasp/encoder/Encode.html
> Which html renderer is used then?
NB uses this for some components like the tree column of the custom tree table:
https://github.com/apache/netbeans/blob/master/platform/openide.awt/src/org/openide/awt/HtmlRenderer.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien merged PR #7128:
URL: https://github.com/apache/netbeans/pull/7128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Git history and diff view improvements [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7128:
URL: https://github.com/apache/netbeans/pull/7128#issuecomment-1987101762
I found a few more issues but thats as much I want to do via a single PR.
File filter is now implemented, this required some optimizations - which is a good thing since this made everything faster.
- the cell renderer code which calculated the longest path of the displayed file paths had O(n²) complexity, this is now linear
- filter results are cached per revision and are invalidated when the search changes
- some other smaller optimizations
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists