C2025-027-AnnotationsTypeSelect #574

Open
plarsen wants to merge 3 commits from C2025-027-AnnotationsTypeSelect into Development
Owner

Develop a way to filter annotations so the user can view only the types they want to see.

Develop a way to filter annotations so the user can view only the types they want to see.
plarsen self-assigned this 2025-07-11 16:10:01 -04:00
djankowski was assigned by plarsen 2025-07-11 16:10:01 -04:00
jjenko was assigned by plarsen 2025-07-11 16:10:01 -04:00
mschill was assigned by plarsen 2025-07-11 16:10:01 -04:00
plarsen added 1 commit 2025-07-11 16:10:01 -04:00
plarsen requested review from jjenko 2025-07-11 16:10:22 -04:00
jjenko requested review from mschill 2025-07-11 16:34:04 -04:00
mschill reviewed 2025-07-14 07:56:27 -04:00
@ -206,7 +206,7 @@ namespace VEPROMS.CSLA.Library
public partial class AnnotationAuditInfoListPropertyDescriptor : vlnListPropertyDescriptor
{
private AnnotationAuditInfo Item { get { return (AnnotationAuditInfo)_Item; } }
public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;}
Owner

Not sure I understand why this change was made? / It appears to be the only change to the file?

Not sure I understand why this change was made? / It appears to be the only change to the file?
Owner

If you want to keep commented out code, you should a comment explaining why to keep it

If you want to keep commented out code, you should a comment explaining why to keep it
Author
Owner

a space was switched at the end of line 209. It is the same code as before. Not sure why that change is there because I did not make it.

Before: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;}

After: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) {; }

a space was switched at the end of line 209. It is the same code as before. Not sure why that change is there because I did not make it. Before: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) { ;} After: public AnnotationAuditInfoListPropertyDescriptor(AnnotationAuditInfoList collection, int index) : base(collection, index) {; }
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:56:58 -04:00
@ -0,0 +1,226 @@
// ========================================================================
Owner

Not sure I understand why a _bak file was created and is being checked in for this?

Not sure I understand why a _bak file was created and is being checked in for this?
Author
Owner

Not sure how this got in GIT. It is no long in the project or on the file system.

Not sure how this got in GIT. It is no long in the project or on the file system.
Author
Owner

Sorry I see the file now. I will remove it.

Sorry I see the file now. I will remove it.
Author
Owner

Then odd thing is that I removed it from the project, but it got included in the pull request.

Then odd thing is that I removed it from the project, but it got included in the pull request.
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:57:59 -04:00
@ -0,0 +67,4 @@
dt.Rows.Add(row);
}
//row = dt.NewRow();
Owner

unless reason for keeping the commented out code --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code --- commented out code shouldn't be checked in? (this would be throughout this file)
Author
Owner

I removed the commented code.

I removed the commented code.
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 07:59:28 -04:00
@ -0,0 +16,4 @@
//namespace VEPROMS.CSLA.Library;
// C2025-027 this new file is used to support (data retrival) for selecting Annotation types to display on the Annotation screen.
Owner

Just to double check ---- Since there are many annotation screens, should we be more specific / say this is related to Annotation type filtering through V->Options?

Just to double check ---- Since there are many annotation screens, should we be more specific / say this is related to Annotation type filtering through V->Options?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:05:16 -04:00
mschill reviewed 2025-07-14 08:10:56 -04:00
@ -0,0 +204,4 @@
throw new DbCslaException("Error on AnnotationTypeInfoList.Get", ex);
}
}
private int _TypeID;
Owner

might be missing something ---- but Not sure I am understanding why these are compile time --- i.e. System.Runtime.CompilerServices.MethodImpl

seams like these would be class specific & if they are, should the methods that reference these not be "static" / i.e. could you end up with strange behavior if those methods are called multiple times (multiple users/open windows of the program)? ---- again, may not be the case, but figured I would double check?

might be missing something ---- but Not sure I am understanding why these are compile time --- i.e. System.Runtime.CompilerServices.MethodImpl seams like these would be class specific & if they are, should the methods that reference these not be "static" / i.e. could you end up with strange behavior if those methods are called multiple times (multiple users/open windows of the program)? ---- again, may not be the case, but figured I would double check?
Author
Owner

This code was borrowed code that is not being used. I may take it out. I have it in the file when I tried to use CSLA DataPortal_Fetch. I have run into too many problems trying to use CSLA. I will give it another try.

This code was borrowed code that is not being used. I may take it out. I have it in the file when I tried to use CSLA DataPortal_Fetch. I have run into too many problems trying to use CSLA. I will give it another try.
Author
Owner

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.
Owner

I would recommend either it should follow CSLA or it should notmainly for 2 reasons:

  1. if it half follows CSLA, then we could run into problems down the line if we upgrade CSLA or replace it with something else
  2. CSLA is a little more bulky / heavy on memory / caching and other things ---- so if we aren't using it for DB access, then (unless there is a specific reason) why introduce the extra overhead.
I would recommend either it should follow CSLA or it should notmainly for 2 reasons: 1. if it half follows CSLA, then we could run into problems down the line if we upgrade CSLA or replace it with something else 2. CSLA is a little more bulky / heavy on memory / caching and other things ---- so if we aren't using it for DB access, then (unless there is a specific reason) why introduce the extra overhead.
mschill reviewed 2025-07-14 08:20:10 -04:00
@ -0,0 +361,4 @@
}
}
private void DataPortal_Fetch(retrieveAnnotSelections criteria)
Owner

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:20:26 -04:00
@ -0,0 +404,4 @@
_itemID = itemID;
}
}
private void DataPortal_Fetch(AnnotationSelectByItemIDCriteria criteria)
Owner

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?

may be missing something .... but if we are doing a "minimal" implementation --- i.e. not using csla (using sqlconnection and createcommand) for DB access, is there a reason we are using csla's DataPortal_Fetch?
Author
Owner

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.

I will remove it. I was initially trying CSLA, but CSLA is not easy to work with. I do not want to waste time to get CSLA to work on minor data requests.
mschill marked this conversation as resolved
jjenko requested changes 2025-07-14 08:21:25 -04:00
jjenko left a comment
Owner

I agree with Matt's comments.
Please removed commented out code or add a comment with reason to keep
Also _bak files should not be checked in.

I agree with Matt's comments. Please removed commented out code or add a comment with reason to keep Also _bak files should not be checked in.
mschill reviewed 2025-07-14 08:30:18 -04:00
@ -123,1 +123,3 @@
}
// C2025-027
//cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(ProcItem.ItemID);
cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID, ProcItem.ItemID);
Owner

this is set to a different DataSource in:
public void SetupAnnotations(DisplaySearch annosrch)

should this be there/replace that?

DisplayMember / ValueMember / etc... is set there - are they the same?

which should be the driving DataSource / (what would happen if a discrepancy between the two - for example if all annotation types were filtered out)?

this is set to a different DataSource in: public void SetupAnnotations(DisplaySearch annosrch) should this be there/replace that? DisplayMember / ValueMember / etc... is set there - are they the same? which should be the driving DataSource / (what would happen if a discrepancy between the two - for example if all annotation types were filtered out)?
mschill marked this conversation as resolved
mschill reviewed 2025-07-14 08:36:43 -04:00
@ -1296,7 +1296,38 @@ namespace VEPROMS
pi.MyDocVersion.DocVersionConfig.SelectedSlave = 0;
}
//void tv_SelectAnnotations(object sender, vlnTreeEventArgs args)
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in? (this would be throughout this file)
mschill marked this conversation as resolved
plarsen added 1 commit 2025-07-15 23:13:13 -04:00
plarsen added 1 commit 2025-07-15 23:31:12 -04:00
mschill requested changes 2025-07-16 10:25:43 -04:00
mschill left a comment
Owner

I re-reviewed this morning.

see Pending comments - let me know if you have any questions.

I re-reviewed this morning. see Pending comments - let me know if you have any questions.
@ -0,0 +14,4 @@
// C2025-027 Annotation Type Filtering
public partial class DlgAnnotationsSelect : Form
{
int AnnotationTypeID;
Owner

just to check - is there a reason these 2 items (the id and NameStr) are global variables / should stay in memory while the form is open?

It looks like they are only used in 1 function - update?

just to check - is there a reason these 2 items (the id and NameStr) are global variables / should stay in memory while the form is open? It looks like they are only used in 1 function - update?
@ -0,0 +25,4 @@
public DlgAnnotationsSelect(string userid)
{
InitializeComponent();
//MyItemID = pi.ItemID;
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

commented out code should be removed or reason given for why it is commented out / should be in the code long term.
@ -0,0 +97,4 @@
// Save selected list to DB.
private void btnUpdate_Click(object sender, EventArgs e)
{
int dltFlg = 1;
Owner

I am not sure what this flag is?
I noticed it will be 1 on 1st iteration then 0 after that.

Can there be comments explaining why?

I am not sure what this flag is? I noticed it will be 1 on 1st iteration then 0 after that. Can there be comments explaining why?
@ -0,0 +101,4 @@
foreach (AnnotataionItem item in lstSelected.Items.OfType<AnnotataionItem>())
{
lstSelected.DisplayMember = "NameStr";
Owner

just to double check --- Is there a reason we are updating the DisplayMember / ValueMember every time we go through the loop?

Wasn't this already set when the form loaded --- line 160ish?

just to double check --- Is there a reason we are updating the DisplayMember / ValueMember every time we go through the loop? Wasn't this already set when the form loaded --- line 160ish?
@ -24078,0 +24129,4 @@
-- Description: Retrieve Current Annotation Types
-- =============================================
CREATE PROC [dbo].[getAnnotationstypeSelections]
Owner

So .... unless I have this backwards:
This is for filtering out Annotation Types --- so wouldn't anything selected in AnnotationTypeSelections need / be filtered out.

so basically, by default, we would show all annotation types / if a new annotation type was added it would show & only what is selected would be filtered out .......

if this is the case, wouldn't we want this info to come from AnnotationTypes
& be basically:

Select
AnnotationTypes.[TypeID],
AnnotationTypes.[Name],
AnnotationTypes.[Config],
AnnotationTypes.[DTS],
AnnotationTypes.[UserID],
AnnotationTypes.[LastChanged],
(SELECT COUNT(*) FROM [Annotations] WHERE [Annotations].[TypeID]=[AnnotationTypes].[TypeID]) [AnnotationCount],
AnnotationTypes.[IsEPAnnotationType]
FROM [AnnotationTypes]
LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID
WHERE AnnotationTypeSelections.UsrID = @UsrID
AND
AnnotationTypeSelections.ASTypeID IS NULL

----again, may be thinking of this wrong ... but was thinking we were storing in the db what they picked to filter out.

So .... unless I have this backwards: This is for filtering out Annotation Types --- so wouldn't anything selected in AnnotationTypeSelections need / be filtered out. so basically, by default, we would show all annotation types / if a new annotation type was added it would show & only what is selected would be filtered out ....... if this is the case, wouldn't we want this info to come from AnnotationTypes & be basically: Select AnnotationTypes.[TypeID], AnnotationTypes.[Name], AnnotationTypes.[Config], AnnotationTypes.[DTS], AnnotationTypes.[UserID], AnnotationTypes.[LastChanged], (SELECT COUNT(*) FROM [Annotations] WHERE [Annotations].[TypeID]=[AnnotationTypes].[TypeID]) [AnnotationCount], AnnotationTypes.[IsEPAnnotationType] FROM [AnnotationTypes] LEFT OUTER JOIN AnnotationTypeSelections ON AnnotationTypeSelections.TypeID = AnnotationTypes.TypeID WHERE AnnotationTypeSelections.UsrID = @UsrID AND AnnotationTypeSelections.ASTypeID IS NULL ----again, may be thinking of this wrong ... but was thinking we were storing in the db what they picked to filter out.
@ -24078,0 +24232,4 @@
-- Description: Store user Annotation selections for annotation filter.
-- =============================================
CREATE TABLE [dbo].[AnnotationTypeSelections](
Owner

Should this table have any other indexes?

Would think that there could be a lot of joins by TypeID and User...

Also, what is the difference between:
UsrID
and
UserID?

Should this table have any other indexes? Would think that there could be a lot of joins by TypeID and User... Also, what is the difference between: UsrID and UserID?
@ -0,0 +1,190 @@

namespace VEPROMS
Owner

Naming/casing should be consistent:
class is:
DlgAnnotationsSelect
but
designer is
dlgAnnotationsSelect?

Naming/casing should be consistent: class is: DlgAnnotationsSelect but designer is dlgAnnotationsSelect?
@ -1346,3 +1404,4 @@
private DevComponents.DotNetBar.Controls.CheckBoxX cbOTRemember;
private DevComponents.DotNetBar.Controls.CheckBoxX cbOTAutoOpen;
private DevComponents.DotNetBar.Controls.CheckBoxX cbShwRplWrdsColor;
//private DevComponents.DotNetBar.ButtonItem cbShwAnnoFilter;
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

may be other places in this file as well - this is just where I noticed it.

commented out code should be removed or reason given for why it is commented out / should be in the code long term. may be other places in this file as well - this is just where I noticed it.
@ -366,3 +366,3 @@
Properties.Settings.Default.Save(); // save settings
}
}
private void cbShwAnnoFilter_Click(object sender, EventArgs e)
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

(this would be throughout this file)

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in? (this would be throughout this file)
Owner

Now, this is a method with everything inside commented out --- if no code is to execute, should the method be removed?

Now, this is a method with everything inside commented out --- if no code is to execute, should the method be removed?
@ -369,0 +376,4 @@
//Properties.Settings.Default.Save(); // save settings
}
private void buttonX1_Click(object sender, EventArgs e)
Owner

Should this be named something other than buttonX1?

Should this be named something other than buttonX1?
@ -544,3 +544,3 @@
tv.ProcedureCheckedOutTo += new vlnTreeViewEvent(tv_ProcedureCheckedOutTo);
tv.ViewPDF += new vlnTreeViewPdfEvent(tv_ViewPDF);
//tv.SelectAnnotations += new (tv_SelectAnnotations);
Owner

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?

unless reason for keeping the commented out code (should be explained in the comments) --- commented out code shouldn't be checked in?
@ -0,0 +41,4 @@
// if the user has not created a annotation sub-set list saved to AnnotationTypeSelections table.
if (dt.Rows.Count < 1)
{
//dt.Rows.Add(DataPortal.Fetch<AnnotationTypeInfoList>());
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term.

commented out code should be removed or reason given for why it is commented out / should be in the code long term.
@ -122,2 +122,2 @@
ProcItem = CurrentItem.MyProcedure;
}
//ProcItem = CurrentItem.MyProcedure;
Owner

just to double check - was this intended to be commented out?

just to double check - was this intended to be commented out?
@ -124,0 +123,4 @@
//ProcItem = CurrentItem.MyProcedure;
// C2025-027 Annotation Type Filtering
//cbGridAnnoType.WatermarkText = "Select Annotation Type";
//cbGridAnnoType.DataSource = VEPROMS.CSLA.Library.AnnotationstypeSelections.Get(MyUserInfo.UserID);
Owner

commented out code should be removed or reason given for why it is commented out / should be in the code long term. (lines 122-127)

commented out code should be removed or reason given for why it is commented out / should be in the code long term. (lines 122-127)
@ -365,6 +370,12 @@ namespace Volian.Controls.Library
cbGridAnnoType.DisplayMember = "Name";
cbGridAnnoType.ValueMember = "TypeId";
cbGridAnnoType.DataSource = AnnotationTypeInfoList.Get().Clone();
Owner

this should be removed if data set is set to something else (see 2 lines after this)

this should be removed if data set is set to something else (see 2 lines after this)
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin C2025-027-AnnotationsTypeSelect:C2025-027-AnnotationsTypeSelect
git checkout C2025-027-AnnotationsTypeSelect
Sign in to join this conversation.
No Reviewers
No Label
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Volian/SourceCode#574
No description provided.